🇧🇷Brazil @renatog

Aware about patch link on #41 however if MR is updated the patch is updates as well

So uploading the same patch in a file to avoid being updated. So it can be used on composer if needed

Sorry @renatog for removing the AJAX URL by mistake

No problems at all

I have added that back and working fine for me please review

Thank you so much! It's really a great start!

However I was thinking about an checkbox in the admin area
Example:
Go to accordion "Don't show again"
A new dropdown called: "Don't show again when:"
Two options:

1. Marked the checkbox and right button clicked (default)
2. Marked the checkbox and Modal closed by any means

The option #1 will continue as today (apply cookie only if checkbox is Marked and clicked in the right button)
If the user selects option #2 when close the Modal the logic on this MR would be applied, you know?

Nice catch! Thanks for reporting

Thanks for your effort on this! It's amazing

But the AJAX url has been removed intentionally?

/modal/ajax/hook-modal-submit

@ytsuhako I'm facing the same issue

aligns with CKEditor 5's behavior

Aware that is from CK side but did you have any solution on that?

Alternative, workaround, etc?

Moved to the dev branch

Thanks everyone

Update

Knowing that the right button allows to redirect to another URL, the important thing should be: Did I check the 'Don't show again' box, whatever button I choose to close the modal

Good catch

There is also a scenario where the end-users can mark the "don't show again" and closing the Modal using the ESC key or clicking outside. So it'll be closed without clicking in right or left button

I think would be great if has an option to consider the "don't show again" on these cases as well

I'm converting this issue into a "feature request" so we can have this idea mapped to be implemented in the future

It's a really interesting point

When created we assumed that the right button was the "OK button" and left button was like a "Cancel button"

For this reason the "Don't show again" was working only with the right button, you know?

However I think it's a interesting feature

Like, inside of configuration of "Don't show again" we can set if it would be applied by right button, left button or maybe both buttons

What do you think, is that makes sense?

Perfect!

So team can use via composer with composer require 'drupal/invite:3.0.x-dev@dev'

Next step we can create a beta version based on 3.0.x

Thank you so much @kensae

Ok, thanks for your effort on this

Moved to the dev branch 3.0.x. Honestly, I didn't finish all tests yet, but I don't want to block anyone on this. So moving to the dev branch can helps some people that want to test via composer, etc

Do you think is necessary anything else on this branch of it's everything done from your side?

does not apply to latest dev branch

So I think we can mark as "Needs Works", right?!
Plan is merge that into 3.0.x branch
If can't be merged now is necessary effort to fix that
So lets mark as NR to track that
If someone needs, can create a patch file to be applied on 3.0.0 via composer
However the MR should be merged into dev branch, so IMHO makes sense mark that to work

Thanks for merging that @jaseerkinangattil

Curiosity: I saw that was committed but the issue status still as "Needs Review". Is there something pending, or we can close as "fixed"?

Merged into dev branch

Thank you so much, everyone

Thanks for your feedback @zigazou

translation of "Add another class" and "Remove last added class" could be included as well

This one has been fixed at 🐛 Some button labels are not translatable Active

Created the merge request pointing to branch 1.0.x just in case it helps to merge easily via UI

https://git.drupalcode.org/project/social_auth_entra_id/-/merge_requests/1

I'll work on the issue branch in this issue?

Can be

Unfortunately I don't have permission to add you as a co-maintainer yet
However, I just created the new branch 3.0.x-dev → specifically for you
Feel free to create MR and patches pointing to this branch
I'll help merging them
When it's finished, we can release a beta-version

Patch for 1.0.2

renatog → created an issue.

I'm a current maintainer (a little away from this project due to busy agenda) but if you want to contribute we can create a branch and I'm available to merge that

Good catch! Makes sense!

Could someone create a merge request with the same fix of this patch, please?

https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr... →

With that we can merge easily though the GitLab UI, you know?

Hello @zigazou, thanks for reporting

Seems that is a duplication of 🐛 Missing div closing tag breaks block config layout Active

On issue above has a merge request. If possible could you confirm if that solution works for you, please?

If yes, feel free to mark as RTBC and we can merge that soon

Best regards

Sure! I’m on it

Ah, sorry @renatog, please rebase MR to 2.x branch

Got it, no problems

It's done!
MR rebased and now the target points to 2.0.x

Released: https://www.drupal.org/project/external_hreflang/releases/2.0.4 →

MR Merged with '8.x-1.x'

No problems at all. Btw, thanks for reporting and providing the patch

I hope that we can commit soon

Best regards

Seems duplicated in favor of 🐛 Failsafe conversion of block_classes_stored Active , isn't?

Seems good

Merged to the dev branch

Thanks everyone

renatog → made their first commit to this issue’s fork.

mr.baileys → credited renatog → .

Really? 🤔

For me it's appearing as mergeable ( ref )

Ready to merge by members who can write to the target branch

Screenshot attached:

It’s expected. Only 3.0.x and 4.0.x are supported and anyone will commit on 8.x-1.x or 2.0.x anymore

Details

• 3.0.x is based on 8.x-1.x
• 4.0.x is based on 2.0.x
• Only 3.0.x and 4.0.x will receive updates
• Old versions won’t receive new commits anymore
• Honestly I understand your point about the message of “unsupported”
• But I can’t mark as “Supported by Maintainers”
• Because is not true, since anyone will update these branches anymore
• And If mark as “supported” it covers security policy
• So would be necessary overhead duplicating effort applying fixing on these old versions
• I’m sorry about the message of “not supported” but it’s the truth
• If someone needs to remove the message My recommendation is updating as mentioned on Matrix - Upgrade Guidance →

* If you are on 2.0.x: Upgrade to 4.0.x
* If you are on 8.x-1.x: Upgrade to 3.0.x

Yes, I think it makes sense for me

I didn't increment the hook_update because i don't have any table called "modal"

But the idea of this hook_update is set the default of this new field for all existent modals, you know?

• So let's imagine that one website has 10 modals
• This field don't exists yet
• After this update, the field will be there
• But should have a "default value" right?
• For example, false, or 0
• So, Maybe we need to create a hook update that:
• A) Will load all Modals
• B) Will set the default value for this new field (false or 0, etc)
• C) Save

Verified that on MR we updated an existing hook_update, right?!

On this case we need to create a new exclusive hook update, because we can have websites running in production that already executed all pending hook updates, and this hook won’t be executed again

So, if possible, I’d suggest creating a new hook updated specifically for this

Amazing! Let’s test it

Thank you so much Vanessa! Really appreciates your contribution

In the function ModalPageService::loadModalsToShow(), is it possible to make a change ?
Change:

$modalsToShow[] = $modalToShow;
by

if($modalToShow)
$modalsToShow[] = $modalToShow;

Yes, it's possible

I'd suggest making this change in your local
If it works feel free to post a patch or Merge Request
We can test for other scenarios
If it works we can merge that into 5.x and 6.x

Interesting

You can have 1 modal on CMS that appears for 2 domains?

Like:

• mydomain_modal1
• mydomain_modal2
• mydomain_modal3

And others

• mydifferentdomain_modal1
• mydifferentdomain_modal2
• mydifferentdomain_modal3

And you have 2 sites using the same CMS?

Like

1. www.mydomain.com
2. www.mydifferentdomain.com

Is that?

How it works? Like multisites or using platform that has one module for 2 sites?

Asking just as a curiosity. Maybe we can think about a feature to allow this natively instead of hook, you know?

Good catch

I think we should update here: https://git.drupalcode.org/project/modal_page/-/blob/5.1.0/modal_page.in...
To ^10.3

Or remove this dependency: https://git.drupalcode.org/project/modal_page/-/blob/5.1.0/modal_page.in...

You're right

I don't remember why we have this dependency on 'drupal:system (>= 10.3)'

Will be necessary investigate better

Anyway, I'm not sure if it's critical. Maybe you can't install Modal, but I wouldn't break the site for example

If you have a different opinion why do you think it's critical, please let me know

Awesome! Really appreciated!

Let's test that

Thank you so much

Thanks for reporting @joseft40

The proposed solutions seems good

I'm busy in the last days but if you're able to provide a patch it'll be very useful

I see in my drupal /admin/modules/update the recommended version 3.0

Yes, probably it's because of sequence number on semver

In the release notes of 3.0.x asks to upgrade from 8.x: https://www.drupal.org/project/block_class/releases/3.0.0 →

Upgrade from 8.x-1.x:
If you are using 8.x-1.x-dev or 8.x-1.4 versions of the module, then you should consider upgrading to respectively 3.0.x-dev and 3.0.0

please then also make it so, that people with the 2.0.12 version see as recommended version 4.0

I marked 4.0.x as recommended by maintainer
Let's see if it helps

I will update to version 4.0 because of the explanation of this thread. Thanks!

Great! My pleasure

just upgraded successfully to v4.0

Amazing! I'm happy to help

thank you for your fast clarification

My pleasure

FYI: Added a Matrix of compatibly and a guidance for the upgrade: https://www.drupal.org/project/block_class#matrix →

Any feedbacks on this guideline feel free to let me know

Hello @nojj

Added the orientation at Matrix of compatibility: https://www.drupal.org/project/block_class#matrix →

Summary:

• Had a confusion on release version
• We had 1.x and 2.x
• 3.x created from 1.x
• Users on 2.x that upgrade to 3.x did a downgrade
• So the recommendation is: if you're on 2.x update to 4.x
• More info at 📌 2.x release vs 3.x release: Which should I use? Active

As agreed, created 4.0.0 based on 2.x

Please don't merge yet, I'm improving a little bit the MR

I'll move as Needs Review when it's done

Please don't merge yet, I'm improving a little bit the MR

I'll move as Needs Review when it's done

Please don't merge yet, I'm improving a little bit the MR

I'll move as Needs Review when it's done

+1 on this -- please make the 2.x version have a bigger number than the downgraded 3.x version!
Ok, I'm on it

maintainer should release a 4.x being just 2.x but with a higher version

I can help on this ☝️

Context:
I'm a maintainer
However a little away from the project in the last months due to busy agenda on my other projects
I wasn't the responsible for creating 3.x from 1.x
But I agree with on this initiative, as mentioned on #4
So if you agree I can go ahead with suggestion #10 and releasing 4.x from 2.x

Maybe we should add ^10.3 on the .info and composer.json as the 10.2.x is not supported

But the check that you introduced is also valid

Yes, the check is like a fallback just to avoid break someone, but agreed that the info update, would be nice

If you can report an issue, will be really appreciated. If not I can do this later

I also added a Matrix in the project page to help with the supported version

Moved to the dev branch

Thanks everyone

Issue above is fixed at 🐛 Render plain image is deprecated Needs work

Tested and it's really working fine

Moved the same fix of 🐛 Problem with version 2.0.3 and drupal 10 Active

After using code from MR I open "/admin/structure/modal/add" and there is a unexpected error

Error: Call to undefined method Drupal\Core\Render\Renderer::renderInIsolation() in Drupal\modal_page\Form\ModalForm->form() (line 118 of /app/web/modules/contrib/modal_page/src/Form/ModalForm.php).

Drupal 10.2.7

Rolling back to NR, because seems not related to this MR

After using code from MR I open "/admin/structure/modal/add" and there is a unexpected error

Error: Call to undefined method Drupal\Core\Render\Renderer::renderInIsolation() in Drupal\modal_page\Form\ModalForm->form() (line 118 of /app/web/modules/contrib/modal_page/src/Form/ModalForm.php).

Thank you for reviewing @renatog

My pleasure! Appreciated your effort on this @eduardo-morales-alberti

Are there any pending issues?

Not from my side. It's ready to be reviewed?

On #7 you marked as "Needs work"
On #8 you updated the MR but the status wasn't updated to "Needs Review" so I assumed that was pending something else
If everything is ok, please update to NR and we can test that and move forward

Merged to the dev branch: 6.0.x

Since it's a considerable milestone we're going to create a beta-version based on 6.x and keep the stable on 5.x

After a time when it's tested by users, we'll make 6.x stable

Thanks everyone, it's amazing

Any news? we tested it, and works properly

Tested and confirmed that it works fine

Do you know if this problem happens in the last stable version as well?

@ex-dj ?

MR is available: https://git.drupalcode.org/project/ept_tiles/-/merge_requests/4

renatog → created an issue.

MR created: https://git.drupalcode.org/project/ebt_tiles/-/merge_requests/4

renatog → created an issue.

Moved to the dev branch

Thanks for the nice catch

If we want to maintain drupal 9, then we should check if renderInIsolation exists, but supporting drupal 9 and 11 is not recommended

Agreed

Tested and works well

Nevermid, I saw that the rest of logic and really makes sense this logic

Hey Eduardo, thanks for your effort helping Modal project

About your update on MR, seems that we can reduce one condition here:

if (isset($variables['page'])) {
  if (!isset($variables['page']['#cache']['tags'])) {
    $variables['page']['#cache']['tags'] = [];
  }

To this, right?

if (isset($variables['page']) && !isset($variables['page']['#cache']['tags'])) {
  $variables['page']['#cache']['tags'] = [];
}

MR created and ready for review: https://git.drupalcode.org/project/ept_tiles/-/merge_requests/3

renatog → created an issue.

Moved to the dev branch: 5.0.x

Thanks a lot everyone

Nice! Thanks for your feedback

I'll try to focus on this

Sorry, I didn't have time to merge that before but it's on my plans

I'll test that and merge as soon as possible

Thanks! Merged into 5.0.x

Merged into the dev branch

Thanks everyone

Added the README.md as suggested on #12

confirmed that loadMultiple() don't works

Appreciated @gslente

Marking as NR

Thanks for that

'#options' => loadMultiple()

I didn't run yet, but this code above really works to get roles? 🤔

made the suggested change of moving (and modifying) the check to skip processing if garage collector is disabled, into hook_cron instead of being within the service itself

Appreciated

Seems really good, but as suggested on #12 I think would be nice if we update the README.md with this functionality

Thanks for sharing your feedback @flyke

Thanks for your contribution

Marking as "Needs Work" because MR #55 has changes non-related to this issue. E.g. README, etc

Moving to "Needs Work" because needs a merge request → or patch

That's a great catch

Thank you so much for reporting

One thing, you can use ternary operator or ?? instead of this construction

Definitely

Makes sense, I'll work on this

MR is created for this: https://git.drupalcode.org/project/ebt_tiles/-/merge_requests/3

renatog → created an issue.

Created:: https://www.drupal.org/project/modal_page/releases/5.0.11 →

renatog → created an issue.