Make the Settings panel toggle button show its keyboard shortcut in its tooltip #65322
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
afercia
added
[Type] Bug
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +1.51 kB (+0.09%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Sep 17, 2024
| const isPostEditorComplementaryArea = [ | ||
| 'edit-post/document', | ||
| 'edit-post/block', | ||
| ].includes( identifier ); |
There was a problem hiding this comment.
Why should this be limited to these areas only?
There was a problem hiding this comment.
@youknowriad to my understanding, the shortcut itself is meant to work only for these areas. It doesn't open the Styles panel, it doesn't open the Plugins panel. As such, the keyboard shortcut should only be shown in the Settings toggle button tooltip. Otherwise, it will show up also in the Styles toggle or any Plugin toggle button.
There was a problem hiding this comment.
It's not the case in my testing for styles at least. The shortcut is passed by the usage of <ComplementaryArea toggleShortcut={ something } /> So if I don't pass it explicitly in my own plugin, it's not rendered and it's not effective. So I think we can probably remove this hardcoded checks.
There was a problem hiding this comment.
@youknowriad I already tested it. And I tested it again now. Here's what happens when removing that condition:
As you can see, the shortcut is shown also in the Jetpack and Yoast SEO button tooltips. I stand corrected that it does not happen for the Styles button.
There was a problem hiding this comment.
For me, this means that we have a bug here
The shortcut shouldn't be associated with the complementary area at all if it's not meant to be used for it. So basically I think we should just remove that line/prop.
There was a problem hiding this comment.
Ok. The only context I found about the plugin sidebar using the shortcut prop is in this comment #20698 (comment) but I'm not sure I fully understand it Cc @jorgefilipecosta when you have a chance coukd you please share some context?
For now, I just removed the prop from PluginSidebar.
youknowriad
approved these changes
Sep 18, 2024
afercia
deleted the
fix/settings-panel-toggle-missing-shortcut-in-tooltip
branch
noisysocks
added
the
Backport to WP 6.7 Beta/RC
github-actions
bot
removed
the
Backport to WP 6.7 Beta/RC
gutenbergplugin
pushed a commit
that referenced
this pull request
Sep 23, 2024
…ts tooltip (#65322) * Make the Settings panel toggle button show its keyboard shortcut in its tooltip. * Do not pass shortcut prop to PluginSidebar and simplify. Co-authored-by: afercia <afercia@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
github-actions
bot
added
the
Backported to WP Core
|
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 9e45522 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #65310
What?
The Settings button doesn't show its keyboard shortcut in its tooltip.
Why?
All buttons that have an associated keyboard shortcut are expected to visually expose the shortcut in their tooltip.
How?
Note:
I would have liked there was a better way to check whether a ComplementaryArea comes from core or plugins. I see that the complementary area scope prop is required and it appears to be used for this purpose. However, in my testing, I found that it isn't reliable.
identifierandscopefor the Jetpack sidebar, the returned values arejetpack-sidebar/jetpack, which is correct, andcorewhich appears to be incorrect. I guess it would be good to inform the Jetpack plugin authors to double check.seo-sidebar.coreas the scope value? I guess this value should be reserved. Plugins should be enforced to use a prefixed, unique, scope.edit-post/documentandedit-post/block. I see similar hardcoded values are already in place when the keyboard shortcut is registered, so I guess it's not terribly bad. However, there should be a better way to check whether a complementary area sidebar comes from core or not. even better, to check if it is associated with a keyboard shortcut. If anyone has better idea to check this, please let me know.Also note:
The Widgets page Settings panel isn't associated with a keyboard shortcut. A decision shoul dbe made on whether to add the shortcut in the first place but that's out of the scope of this PR.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Before:
After: