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

[4.0] Xtd modals done right #33216

Merged
merged 8 commits into from Apr 22, 2021

Conversation

dgrammatiko
Copy link
Contributor

@dgrammatiko dgrammatiko commented Apr 21, 2021

Pull Request for Issue #33210 .

Summary of Changes

  • Modals should not break when debug language is on
  • Modals should have unique ids (since there is an option for multiple instances of an editor)

Testing Instructions

  • Enable debug and the language debug and also the display (the 2 switches that appear after enabling debug)
  • Switch between Tinymce, Codemirror and none editor in the Global configuration
  • Create an article and test ALL the xtd-buttons

Actual result BEFORE applying this Pull Request

Broken

Expected result AFTER applying this Pull Request

Fixed

Documentation Changes Required

NO

~~Actually, yes! The PR #19789 from 2018 introduced the property realName which basically is the untranslated string for the button and this is a B/C break and although buttons without it should still work the J4 implementation of the buttons should dictate for it for a11y and also to ensure proper functionality of the multiple instances of a browser. ~~

Rework the code so it should be B/C although J4 XTD Buttons should have a property name and another icon to ensure there won't be accidental conflicts

@richard67
Copy link
Member

Appveyor failure is not related to this PR.

@infograf768
Copy link
Member

I have tested this item ✅ successfully on e65da44


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

@infograf768
Copy link
Member

I have tested this item ✅ successfully on 55e6ca9


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

@Quy
Copy link
Contributor

Quy commented Apr 22, 2021

I have tested this item ✅ successfully on 3c89ba0


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

1 similar comment
@infograf768
Copy link
Member

I have tested this item ✅ successfully on 3c89ba0


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

@infograf768
Copy link
Member

rtc


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Apr 22, 2021
@richard67
Copy link
Member

@dgrammatiko Is the "Documentation Changes Required" still up to date, i.e. it needs documentation changes? Or has that become obsolete after your recent changes? I ask because if it needs documentation changes, I have to set the corresponding label before merging, so it later won't be forgotten.

@dgrammatiko
Copy link
Contributor Author

Or has that become obsolete after your recent changes?

It should be B/C unless I got totally wrong. Let me explain: I introduce here a new property icon but the fallback is the property name (as it was before): $icon = ($button->get('icon')) ? $button->get('icon') : $button->get('name');
That said, it should be communicated that ALL the J4 xtd-button SHOULD use these properties not only for the error reported with the debug language strings but mainly because the button MIGHT NOT work as expected when there are multiple instances of editors in a page (eg using custom fields)

@richard67
Copy link
Member

So does it need doc changes or not? That's still not clear to me. A simple "yes" or "no" is sufficient for me, any answer will not block RTC.

@dgrammatiko
Copy link
Contributor Author

dgrammatiko commented Apr 22, 2021

So does it need doc changes or not?

Yes, If https://docs.joomla.org/Special:MyLanguage/Editors_form_field_type this is all the docs about the editor field someone should totally rewrite that and include also the xtd-buttons and the expected properties for the button.

@richard67 richard67 merged commit 16f08f1 into joomla:4.0-dev Apr 22, 2021
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Apr 22, 2021
@richard67
Copy link
Member

Thanks!

@dgrammatiko dgrammatiko deleted the xtd_modals_done_right_4.0-dev branch April 22, 2021 17:03
@richard67 richard67 added this to the Joomla 4.0 milestone Apr 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Required NPM Resource Changed This Pull Request can't be tested by Patchtester
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants