-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Shortcuts: add Ctrl+Y for redo on Windows #42627
Conversation
Size Change: -62 B (0%) Total Size: 1.26 MB
ℹ️ View Unchanged
|
@ellatrix thanks so much again for addressing this issue. I'm not sure how we could test the changes, but the code at least looks good to me! |
I have tested it and it seems to work fine. d9d004bac4665d903adeaaaa5b51b6e4.mp4 |
I think you can test at http://gutenberg.run/42627. |
I have tested again at gutenberg.run, and it looks good to me 👍 |
Works on our end! Thanks very much again @ellatrix! |
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.
Will be nice for Windows users to have this fixed after so long, thanks!
Some testing on Linux might be an idea, since the condition is ! isApple
.
@@ -144,6 +144,8 @@ export const SHIFT = 'shift'; | |||
*/ | |||
export const ZERO = 48; | |||
|
|||
export { isAppleOS }; |
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.
Not completely convinced about exporting this from keycodes
. Maybe there should be a @wordpress/platform
package or something.
It's a very minor thing though, so I'll leave it up to you to decide what to do 👍
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.
Yes, I was wondering also if it's a good idea. But I looked through all the apple checks we're making in Gutenberg, and they're always related to the keyboard, so it seems to make sense as part of this package.
Right, I think for Linux it's ok to make both shortcuts available, as we do for Windows.
See https://en.wikipedia.org/wiki/Control-Y. I'll add an inline comment. |
* | ||
* @return {boolean} True if MacOS; false otherwise. | ||
*/ | ||
function isAppleOS( _window = window ) { |
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.
@talldan I reused the export form the key codes package for these two other instances. As you can see we're always making this check in the context of key codes, so I think it makes sense in this package.
f0ad11f
to
1d897e2
Compare
What?
Fixes #8921.
Why?
Ctrl+Y
is the de-facto default shortcut for undo on Windows.How?
Exports
isAppleOs
in the key codes package because it seems generally handy in the context of shortcuts.Testing Instructions
Check if
Ctrl+Y
works on Windows. Make sure it doesn't work on Mac.Screenshots or screencast