🇧🇷Brazil @renatog

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.

Moved to the dev branch

Thanks

Implemented

Nice catch, thank you so much

We need to fix this line
Your fix makes sense
Moving to "Needs Work" because we just need a MR or a patch
After that I can merge that for us

It's done! Implemented the same solution of EPT Tiles ✨ Add Optional "Target" for Link, with that editors will be able to configure links to open in a new window/tab Fixed

MR is ready: https://git.drupalcode.org/project/ebt_tiles/-/merge_requests/1

MR created with the implementation

renatog → created an issue.

MR created with the fix: https://git.drupalcode.org/project/ebt_basic_button/-/merge_requests/24

renatog → created an issue.

Moved to the dev branch - Thanks

simple but legit, thanks for the fix

Hello @levmyshkin , I hope you're well

Sorry by the delay, I've been busy in the last few days

I suggest to use EptSettingsTilesWidget.php form and add the new field there. You can pass EBT Settings values in hook_preprocess_paragraph(), get parent entity from EPT Tile and save value for Open in a new tab in $variables

Implemented suggestion above

MR is available for review: https://git.drupalcode.org/project/ept_tiles/-/merge_requests/2

amateescu → credited renatog → .

I can confirm that it is working properly. 1 for RTBC!

Great! Thanks for testing

Moving that to RTBC to be merged soon

Nice catch. Thanks for the MR

Added thread above there

Wow that's great!

I think the MR is ready for be reviewed, right?

Marking as "NR", but if isn't ready yet, feel free to update the issue status

Thanks for the confirmation, updating the version

@cyberm what's the version of Modal, please?

nice catch!

Probably has attributes on area that isn't being displayed

Thanks for reporting

Issue is tagged with version: 4.1.18

Did you test using the last stable version? (currently: 5.0.10)

Wow! Seems awesome! Thank you so much @zeshanziya

Moving to NR so we can validate. Anyone are welcome to help testing as well

If it works fine we can create 5.1.0-beta1 with this

Hello Ivan

Did you make it granulary for tile on purpose?

Not actually, It was the initial solution on mind, but didn't think about one checkbox per group

I thought about using one checkbox for entire EPT Tiles which will be applied for all tiles together

Agreed. It'll guarantee consistence across all items

I suggest to use EptSettingsTilesWidget.php form and add the new field there. You can pass EBT Settings values in hook_preprocess_paragraph(), get parent entity from EPT Tile and save value for Open in a new tab in $variables. What do you think?

Makes sense for me
I'll update the MR and I'll let you know

I'll update on EPT first, and when is approved I'll replicate on EBT Tiles ✨ Add the target option to open link on new tab/window Needs work

Merged into the dev branch

seems good

Implemented: https://git.drupalcode.org/project/ebt_tiles/-/merge_requests/1

MR updated: https://git.drupalcode.org/project/ept_tiles/-/merge_requests/2

Merge request is pointing to 8.x-1.x
However issue is tagged to 2.0.1
I'd suggest using 2.x version
So the target branch must updated on MR

checkbox "Open in a new tab" instead of select list for _target attribute. Many people don't know about HTML

Makes sense, totally agree

Assigned to me and I'm going to updating here and apply the same on EPT to keep consistent at ✨ Add Optional "Target" for Link, with that editors will be able to configure links to open in a new window/tab Fixed

Thanks @levmyshkin

Done. MR available at: https://git.drupalcode.org/project/ebt_tiles/-/merge_requests/1

Fixed: https://git.drupalcode.org/project/ebt_tiles/-/merge_requests/2

renatog → created an issue.