-
Notifications
You must be signed in to change notification settings - Fork 8
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
Populate Pantheon HUD menu with an AJAX request on hover #55
Conversation
Hi @danielbachhuber, is there an existing multidev site that I can test and verify this works? Or do I need to create my own wp site and install the plugin :D (first time testing something like this) thank you! |
inc/class-toolbar.php
Outdated
jQuery('#wp-admin-bar-pantheon-hud').on('hover', function() { | ||
if (jQuery('#wp-admin-bar-pantheon-hud-wp-admin-loading').length) { | ||
jQuery.get('{$request_url}', function(data) { | ||
jQuery('#wp-admin-bar-pantheon-hud .ab-sub-wrapper').html(data); | ||
}); | ||
} | ||
}); | ||
EOT; |
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.
This presumes that jQuery is on the page, but this is not necessarily the case. In fact, Twenty Nineteen and Twenty Twenty don't use jQuery at all and so it is not enqueued on the page. Core recently improved its handling of the admin bar when jQuery is not present: https://core.trac.wordpress.org/ticket/47069
So this needs to be rewritten to use vanilla JS.
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.
@westonruter Great suggestion! updated with 5305ce1
inc/class-toolbar.php
Outdated
admin_url( 'admin-ajax.php' ) | ||
); | ||
$script = <<<EOT | ||
jQuery('#wp-admin-bar-pantheon-hud').on('hover', function() { |
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.
Also, this needs to take into account keyboard navigation (e.g. it should trigger on focus
as well).
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.
Included with 5305ce1
Co-Authored-By: Weston Ruter <westonruter@google.com>
@sugaroverflow Sorry for the late reply here. I wanted to address @westonruter's comments before you checked the changes out. The plugin isn't auto-deployed to a multidev environment right now. You should be able to install it on any old Pantheon WordPress site though. |
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.
Thanks @danielbachhuber for showing me how to test this on a Wordpress site - super appreciated! I was able to get this working, the code looks good to me, and @westonruter also did a code review - so this sounds good to go 🎉 👍
Thanks @sugaroverflow ! |
Highly effective! |
Fixes #18
If you'd like to test it locally, drop this code snippet into a mu-plugin to mock a bunch of data: