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] Remove closest and matches polyfills #28695

Merged
merged 4 commits into from
Apr 16, 2020
Merged

[4.0] Remove closest and matches polyfills #28695

merged 4 commits into from
Apr 16, 2020

Conversation

C-Lodder
Copy link
Member

Pull Request for Issue #25186

Summary of Changes

This removes the custom polyfills for the closest() and matches() methods and replaces with the the native JS methods

Testing Instructions

  1. Ensure you can unpublish a module in the admin dashboard and it's removed
  2. Ensure subform fields work as expected
  3. Ensure the sidebar menu works the same as before

Or:

Code review

@wilsonge wilsonge merged commit 1e46ca3 into joomla:4.0-dev Apr 16, 2020
@wilsonge
Copy link
Contributor

Thanks!

@wilsonge wilsonge added this to the Joomla 4.0 milestone Apr 16, 2020
@C-Lodder C-Lodder deleted the js-stuff branch April 16, 2020 13:37
@C-Lodder
Copy link
Member Author

@wilsonge Just an FYI, closest() isn't supported in IE11, so if you still expect frontend JS to work in IE11, then you may want to revert this.

@Quy
Copy link
Contributor

Quy commented Apr 21, 2020

To reproduce, go to Global Configuration > Users > Email Domain Options, click + icon.

TypeError: row is null

/media/system/js/fields/joomla-field-subform.js:107

row = row.closest('joomla-field-subform') === that ? row : null;

@wilsonge
Copy link
Contributor

@Quy what browser is that in? I'm not too fussed about dropping IE11 support I don't think at this point.

@Quy
Copy link
Contributor

Quy commented Apr 21, 2020

Firefox 75.0 Windows 10

@C-Lodder
Copy link
Member Author

@Quy I'll look into this now anf fix. Thanks for reporting.

@wilsonge Can you please make a dedicated decision about IE11

@C-Lodder
Copy link
Member Author

@Quy #28753

@wilsonge
Copy link
Contributor

Started going through the full requirements list two weeks ago to get formal approval from production. Right now my intention is no ie11 support. But that’s still to be fully confirmed based on their vote

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

4 participants