Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[6.x] Use Dialog for Article links, when "modal option" is selected #42461

Merged
merged 7 commits into from
Mar 5, 2024

Conversation

Fedik
Copy link
Member

@Fedik Fedik commented Dec 4, 2023

Summary of Changes

Use Dialog for Artcile links, when "modal option" is selected.

And new feature: Allow to show inline content in the popup.

Testing Instructions

Add a couple links to the Article,
First link: any regular URL.
Second link: #my-popup-target.
Also add to the article content, following html:

<div class="hidden">
<div id="my-popup-target">
<p class="p-3">Lorem ipsum dolor sit amet, cu vis velit signiferumque, eos no possim mollis facilisi.</p>
</div>
</div>

For both links select option: show in modal.

Got to article view, and click these links.

Actual result BEFORE applying this Pull Request

First link works in BS Modal:
Screenshot 2023-12-03_17-59-26
Second link does nothing.

Expected result AFTER applying this Pull Request

First link works with Joomla Dialog:
Screenshot 2023-12-04_11-07-49

Second link works with Joomla Dialog:
Screenshot 2023-12-04_11-21-21

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:
  • No documentation changes for docs.joomla.org needed
  • Pull Request link for manual.joomla.org: TBD
  • No documentation changes for manual.joomla.org needed

References:

@Fedik Fedik requested a review from chmst as a code owner December 7, 2023 08:36
@fgsw
Copy link

fgsw commented Dec 9, 2023

Test successfully but can't mark it on issues.joomla.org, get Composer detected issues in your platform: Your Composer dependencies require a PHP version ">= 8.1.0".

@Quy
Copy link
Contributor

Quy commented Dec 10, 2023

I have tested this item ✅ successfully on 7801a6a


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42461.

@Quy
Copy link
Contributor

Quy commented Dec 10, 2023

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42461.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Dec 10, 2023
@LadySolveig
Copy link
Contributor

Thank you @Fedik for this nice work!

A direct change of the modal option for the links in article could lead to broken styles and functions in the frontend for some users in conjunction with a Cassiopeia child template, for example.
If a template uses the bootstrap modal events with custom javascript to perform another action when opening or closing, this will unexpectedly stop working. Similarly, custom styles for the modals have to be rewritten from the users and custom css for this purpose will unexpectedly not work anymore.

Perhaps another select option for the new dialog would be a variant to prevent this.
And the deprecation of the BS modal in the frontend for the article links could be the long-term solution.

What do you think about this?
Would be glad to hear further opinions as well.

@Fedik
Copy link
Member Author

Fedik commented Jan 5, 2024

I can do another option, it is not a problem for me.
But I have seen that current modal option looks bad anyway.

What other thinks? :)

@Fedik
Copy link
Member Author

Fedik commented Jan 6, 2024

On second thought, another "modal" option will be very confusing, we can probably close it and keep BS here, or rebase to 6-dev

@LadySolveig
Copy link
Contributor

LadySolveig commented Jan 6, 2024

On second thought, another "modal" option will be very confusing

That's exactly what bothers me a little. But I think the new dialog looks much better by default even without adjusting the css. And can be used out of the box with a template that ships no bootstrap. Tricky :) I will try if I can get a few more opinions on this.

@LadySolveig
Copy link
Contributor

@Fedik thank you for your work. We decided to rebase to 6

@LadySolveig LadySolveig added PR-6.0-dev and removed PR-5.1-dev RTC This Pull Request is Ready To Commit labels Jan 11, 2024
@joomla-cms-bot joomla-cms-bot added PR-5.1-dev RTC This Pull Request is Ready To Commit labels Jan 11, 2024
@LadySolveig LadySolveig changed the base branch from 5.1-dev to 6.0-dev January 11, 2024 21:48
@Quy Quy removed the PR-5.1-dev label Jan 11, 2024
@laoneo laoneo added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Jan 18, 2024
@Fedik Fedik changed the title [5.1] Use Dialog for Article links, when "modal option" is selected [6.x] Use Dialog for Article links, when "modal option" is selected Jan 24, 2024
@Quy Quy added Updates Requested Indicates that this pull request needs an update from the author and should not be tested. and removed RTC This Pull Request is Ready To Commit Updates Requested Indicates that this pull request needs an update from the author and should not be tested. labels Jan 24, 2024
@Hackwar
Copy link
Member

Hackwar commented Feb 21, 2024

Can you please make the requested change, so that we can test this during PBF? Then we can already merge this into 6.0.

@Hackwar Hackwar added the PBF Pizza, Bugs and Fun label Feb 21, 2024
@Fedik Fedik removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Feb 21, 2024
@SarahBerryman12
Copy link

I have tested this item ✅ successfully on b1872ff


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42461.

1 similar comment
@eddiekonczal
Copy link

I have tested this item ✅ successfully on b1872ff


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42461.

@richard67 richard67 added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Feb 24, 2024
@richard67
Copy link
Member

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42461.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 24, 2024
@richard67 richard67 removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Feb 24, 2024
@brianteeman
Copy link
Contributor

How can this have been tested with 6.x

@laoneo
Copy link
Member

laoneo commented Feb 26, 2024

@brianteeman the pr was made against the 6.0-dev branch. This is the branch where we commit all the changes which should go into 6.0.

@laoneo laoneo enabled auto-merge (squash) March 5, 2024 13:06
@laoneo laoneo added this to the Joomla! 6.0.0 milestone Mar 5, 2024
@laoneo laoneo merged commit 0fb6c08 into joomla:6.0-dev Mar 5, 2024
3 checks passed
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 5, 2024
@Fedik Fedik deleted the popup-artcile-links branch March 5, 2024 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature PBF Pizza, Bugs and Fun PR-6.0-dev
Projects
None yet
Development

Successfully merging this pull request may close these issues.