Fix save, undo and redo keyboard shortcuts in navigation editor #28257
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
…ved to upper layer so both - save button and cmd+s shortcut are using the same piece of code.
grzim
requested review from
adamziel,
draganescu,
kevin940726,
noisysocks and
tellthemachines
as code owners
|
Size Change: +2.45 kB (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
talldan
reviewed
Jan 18, 2021
There was a problem hiding this comment.
Something I didn't realise is that there is already some code for the shortcut:
gutenberg/packages/edit-navigation/src/components/layout/shortcuts.js
Lines 13 to 22 in b864c5d
gutenberg/packages/edit-navigation/src/components/layout/shortcuts.js
Lines 49 to 57 in b864c5d
Sorry for that, I just thought they hadn't been implemented. The existing implementation uses the useShortcut and registerShortcut functions, the idea behind that APIs is that someday shortcut customisation can be added as a feature.
None of these shortcuts seem to be working though, so it'd be good to figure out why and fix them.
|
It is not working as |
|
@grzim I'm ok with only using hooks. I'm not completely sure why it's a component, but I think other editors also implement it this way using shortcuts, and that was likely copied in the navigation editor. |
|
@talldan I have made a little investigation and it is just like you said. Editors implement it this way in order to make it possible to add shortcuts to blocks like this: The key concept is to make it possible to nest |
…s. Use this mechanism
talldan
approved these changes
Jan 21, 2021
There was a problem hiding this comment.
So here we can either use hooks as this would be a cleaner solution or leave it as it is now what makes it less clean but consistent with other editors. In my opinion - consistency wins here.
Agreed. Could always explore refactoring it all to hooks in a separate PR.
I recall another potential issue with the Register components is that they always have to come before the actual shortcut component, so definite scope for improvement.
Anyway, This is working nicely, I rebased locally to bring in some other recent fixes and can confirm save, undo, and redo are now working well. Thanks @grzim 🎉
talldan
changed the title
talldan
added
[Feature] Navigation Screen
[Type] Bug
This was referenced Mar 11, 2021
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.
Saving post logic moved to upper layer so both - save button and cmd+s shortcut are using the same piece of code.
Description
Regarding #28213 keyboard shortcut to save navigation added.
The listener is added to the editor. As the editor does not play any interactive role, I have disabled the linter rule
eslint-disable-next-line jsx-a11y/no-static-element-interactionsHow has this been tested?
Tested manually
Types of changes
New feature
Checklist: