-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Remove jQuery in the frontend for Twenty Twelve. #1683
base: trunk
Are you sure you want to change the base?
Conversation
@@ -3,6 +3,40 @@ | |||
* accessibility for submenu items. | |||
*/ | |||
( function() { | |||
function getParentElements( element, selector ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -3,6 +3,40 @@ | |||
* accessibility for submenu items. | |||
*/ | |||
( function() { | |||
function getParentElements( element, selector ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
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.