Remove jQuery in the frontend for Twenty Twelve.#1683
Conversation
| * accessibility for submenu items. | ||
| */ | ||
| ( function() { | ||
| function getParentElements( element, selector ) { |
There was a problem hiding this comment.
are these useful enough to be moved into a package for re-use? thinking if we are encouraging the community to transition away from jQuery, covering the top use cases in packages (either distinct - or perhaps bundled into a single wp-dom package for efficiency - would make sense vs repeating the same boilerplate code in each theme.
There was a problem hiding this comment.
I agree, it would be great to have a core-provided script with useful utility functions.
Most of what jQuery offers is nowadays not necessary, but it still has a few nice functions which replicating all the time is painful. Yet, loading jQuery for those is overkill. @mtias Is a simple DOM utility functions package (e.g. wp-dom) something we could consider moving forward through Gutenberg?
There was a problem hiding this comment.
The risk of a DOM utility functions package is that it ends up growing large itself, which is a concern when the current library approach isn't modular.
In the case of themes, given that generally only one of them is ever in use for a single page view, I'd argue that we should keep these functions inline, as currently done in this PR.
There was a problem hiding this comment.
Woohoo, this is great! 🎉
I only just came across these PRs, as I've been meaning to file some myself for a few of these themes. Rewriting jQuery-dependant code can end up leading to some nice improvements, given that jQuery is usually loaded as part of the critical path.
I left some comments and suggestions below.
| * accessibility for submenu items. | ||
| */ | ||
| ( function() { | ||
| function getParentElements( element, selector ) { |
There was a problem hiding this comment.
The risk of a DOM utility functions package is that it ends up growing large itself, which is a concern when the current library approach isn't modular.
In the case of themes, given that generally only one of them is ever in use for a single page view, I'd argue that we should keep these functions inline, as currently done in this PR.
|
Adding a note that after these changes, the JS will no longer work correctly in IE 11. I'm not sure whether this is a concern, given that on the one hand we've deprecated it, but on the other hand we may want to preserve this theme working correctly in older versions of WordPress and their supported browsers. |
|
@sgomes Thanks for the review! I've updated the code accordingly. |
sgomes
left a comment
sgomes
left a comment
There was a problem hiding this comment.
Thank you, @felixarntz! The code looks good to me now!
I haven't had a chance to test it, though, and likely won't any time soon, I'm afraid. Still, I don't want to block progress here, so I'll approve ahead of testing 👍
|
This branch lives on a fork that can no longer be updated, so the trunk conflicts here can't be resolved in place. A refreshed version of this PR, merged with current trunk and with the review feedback above incorporated, is now up at #12411. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @felixarntz@google.com. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
3 participants
Trac ticket: https://core.trac.wordpress.org/ticket/54171
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.