-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Add remaining editable shortcuts #4411
Conversation
Oh, these could actually use something in the tooltip! |
I don't know if that lost commit makes sense. Can always remove it, or @jasmussen, feel free to edit. :) |
@@ -19,22 +19,22 @@ const { ESCAPE, LEFT, RIGHT, UP, DOWN } = keycodes; | |||
const FORMATTING_CONTROLS = [ | |||
{ | |||
icon: 'editor-bold', | |||
title: __( 'Bold' ), | |||
title: __( 'Bold — ⌘B' ), |
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.
Should we use ${ __( 'Bold') } — ⌘B
instead? As we are not expecting keyboard shortcuts to be changed during translation, this way translations for 'Bold' get reused.
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.
Good point, though I'm not sure dash and order should not be translatable. Maybe we can just omit the shortcut itself from the translation.
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.
Should we use ${ __( 'Bold') } — ⌘B instead?
Alas, this wouldn't work for RTL languages like Hebrew and Arabic.
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 think it would be safe to use something like ${ __( 'Bold' ) } ${ __( '-' ) } ⌘B. This way translations are reused and we address the point raised by @iseulde.
Regarding the RTL point, It's a nice question, honestly, I don't know if in RTL normally the shortcuts appear inverted, but in LTR, if the shortcuts appeared like "⌘B — Bold" it would be ok so maybe in RTL both ways are also "ok". To be safe be on the safe side we can use the original version, I don't have a strong opinion here.
This is working really well for me. Except for access+c for code, I can't get that to work for some reason. Mac/chrome. I love that you added the keyboard shortcuts to the tooltips, starting work on #2980: However can we make it look like this instead? If not, then we should replace the — with a couple of spaces (like Is this doable? If not, then probably good to keep the shortcuts still, then we can look at improving the look in a separate PR. |
@jasmussen Updating the PR, but now wondering what colour you have in mind? :) |
@jasmussen I added it as a separate attribute on the toolbar/iconbutton/tooltip, so it can be styled all the same. Let me know if this is what you have in mind, and feel free to change to styles of course. :) Thank you! |
Ah, I missed your comment now, I'm so slow. |
Updated. :) |
I can't reproduce. Are you sure it's the right combination? I also found myself trying cmd+x before, which is cut. :D |
This looks amazing to me. @jorgefilipecosta does this particular layout fix issues with RTL?
I think the reason it's not working is that I have that keyboard shortcut assigned to the magnifier in OSX. If it works for another Mac user then 👍 👍 from me :D |
6fff91c
to
ca593c5
Compare
Rebased. What's left here? |
Took it for a quick spin. Experience wise, this feels solid to me. I don't know if we need any more code reviewing. I would be happy to get this merged in sooner rather than later. If it needs a code review, @youknowriad can you take a look? Looks good to me. |
@@ -19,21 +19,25 @@ const FORMATTING_CONTROLS = [ | |||
{ | |||
icon: 'editor-bold', | |||
title: __( 'Bold' ), | |||
shortcut: '⌘B', |
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'm concerned about the hard-coding of the modifier key as ⌘
, which is meaningless to non-macOS users. IMO, this is a blocker.
There's precedent of cross-platform handling:
gutenberg/edit-post/keyboard-shortcuts.js
Lines 1 to 2 in ca593c5
const isMac = window.navigator.platform.toUpperCase().indexOf( 'MAC' ) >= 0; | |
const mod = isMac ? '⌘' : 'Ctrl'; |
@@ -16,6 +16,7 @@ import { prependHTTP } from '@wordpress/url'; | |||
/** | |||
* Internal dependencies | |||
*/ | |||
import { primaryShortcut, primaryAltShortcut } from 'utils/keycodes'; |
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.
What's primaryAlt
? So primary but not really? So secondary? 🤔
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.
Oops, it should totally be secondary.
utils/keycodes.js
Outdated
* | ||
* @return {string} The keyboard shortcut. | ||
*/ | ||
export function metaShortcut( character, _isMacOS = isMacOS ) { |
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.
What will this be used for?
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.
If we want keyboard shortcuts using Ctrl+Q
on Mac, though admittedly I don't see any yet. It'd be Windows+Q
on Windows… should I just remove it?
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.
Yeah, I think across the whole application, we should only ever implement two combinations: primary and access.
utils/keycodes.js
Outdated
* | ||
* @return {string} The keyboard shortcut. | ||
*/ | ||
export function primaryAccessShortcut( character, _isMacOS = isMacOS ) { |
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.
Could we drop this combination in favour of the standard access combination that is now called accessShortcut
?
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.
That'll change an existing keyboard shortcut: https://github.com/WordPress/gutenberg/blob/add/editable-shortcuts/edit-post/keyboard-shortcuts.js#L8
Which I presume has many modifiers as it isn't common. But we could call it tertiary. I am now realising I made a terrible API by surfacing its underlying implementation.
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 think we should change it be an access shortcut. It was just a random combination that didn't clash with anything.
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.
See #3755 (comment) for history on that.
No particular reason for that combination, it should be revised.
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.
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.
Would be nice to have a combo that is the same on all OS 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.
Ancient history: https://core.trac.wordpress.org/ticket/29558
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.
Oddly enough the Mu doesn't appear outside of Gutenberg. If I type Ctrl+Alt+M into this text field it seems like an invisible unicode character is inserted instead.
Looks like Cmd+Alt on Mac will work, though Cmd+Alt+M is also the shortcut to enable responsive design mode in Firefox so that's a bit annoying. 😆
Maybe this needs to stay a three-character combo, just to be safe?
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.
If there's one clash in that namespace, there's likely to be more. Would be nice to find a maximum two-modifier namespace that can be used in WordPress.
I wonder if we can adopt one of https://en.wikipedia.org/wiki/Access_key...
Curiously enough it list Ctrl+Alt
as reserved on Firefox on Mac for accesskey
, so I wonder why it decided to use that.
I'm fine with just picking one non primary combination in this PR and revise it later.
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 if we change it at all, we can't use access
in TinyMCE: https://github.com/tinymce/tinymce/blob/90a829b4ed7655d91f07df15056a805fa43d49f9/src/plugins/help/main/ts/data/KeyboardShortcuts.ts#L14
format: 'italic', | ||
}, | ||
{ | ||
icon: 'editor-strikethrough', | ||
title: __( 'Strikethrough' ), | ||
shortcut: primaryAltShortcut( 'D' ), |
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 should be accessShortcut
, right? https://github.com/WordPress/gutenberg/pull/4411/files#diff-766381e792acf9d4c2505f3aeddcca7eR181
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.
Arg, yes, thanks, I think all these code names have confused my brain. Sorry! 😓
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.
Yeah, that's why I think two is enough for now. :) #4411 (comment)
11fbe65
to
e6e1e7b
Compare
e5a48d1
to
c62cd5f
Compare
I kept the codes as they were before but added the keycode/shortcut text functions that work across platforms. I tested the key combos here and they worked in Windows/Mac/Linux, Edge/Firefox/Chrome/Safari. It would be nice to simplify the shortcuts but I don't think there are simpler ones we can use that won't cause an error somewhere. 😢 So I think this is ready to go 😄 |
utils/keycodes.js
Outdated
const isMac = _isMacOS(); | ||
|
||
const alt = isMac ? '⌥' : 'Alt'; | ||
const meta = isMac ? '⌃' : '⊞'; |
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.
Just to note: I know this is a common way to display shortcuts on the Mac, but my built-in keyboard doesn't have any of these symbols, so this always confuses me. 🙈
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.
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 don't mind keeping the command symbol since it's still on the keyboard (and I know that one), the rest I'm not so sure. I don't know how people make sense of the shortcuts shown on the Mac menus... :)
When I see ⌥
or ⌃
I just make a guess until I get what I want hahaha.
So... it's up to you. I just wanted to make a note here it may not make sense to some people.
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 sure if someone will try to press the caret key. :D
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.
Yeah I was about to file an issue and ask people what they think, but I say we eschew Mac conventions here for usability. I'll keep the command glyph but change the rest to text. 👍
|
||
export const ALT = 'alt'; | ||
export const CTRL = 'ctrl'; | ||
export const PRIMARY = 'mod'; |
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.
Minor: should this become 'primary'? Not that it matters. :)
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.
So I noticed "mod" was actually used as the key code in https://github.com/WordPress/gutenberg/pull/4411/files#diff-cb4a97a77964e94dc29c6d9caf74f9ccL6... I figured it was good to internally keep it as "mod".
@tofumatt I can't approve my own PR (Github doesn't let me), but it looks good to me! |
💥💥💥💥💥 |
isEditingLink: false, | ||
settingsVisible: false, | ||
opensInNewWindow: !! nextProps.formats.link && !! nextProps.formats.link.target, | ||
newLinkValue: '', | ||
} ); | ||
} | ||
|
||
this.setState( { | ||
isAddingLink: !! nextProps.formats.link && nextProps.formats.link.isAdding, |
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 change is causing issues to the format toolbar. When the modal is renrendered (it can happen when you move the mouse over a block), it disappears if you just clicked the link button.
That's because isAddingLink
is a state value controlled from within this component without changing the formats
on the RichText component.
@@ -10,3 +20,117 @@ export const DOWN = 40; | |||
export const DELETE = 46; | |||
|
|||
export const F10 = 121; | |||
|
|||
export const ALT = 'alt'; |
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.
Every other key exported in this module is its key code, yet for this and other modifiers we export a string? Even though Alt has a code (18)?
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 think this comes from @tofumatt.
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 believe it's used to pass to mousetrap and tinymce which wouldn't accept the codes?
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.
Oops, I should have at least left a comment. I honestly forget at this point; it could have just been terrible newbie-ness. 😅
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.
These are a public API of @wordpress/keycodes
, which describes exports as:
Contains keycodes constants for keyboard keys like DOWN, UP, ENTER, etc.
https://www.npmjs.com/package/@wordpress/keycodes
Yet...
import * as keycodes from '@wordpress/keycodes';
keycodes.DOWN // 40 ✅
keycodes.UP // 38 ✅
keycodes.ENTER // 13 ✅
keycodes.ALT // 'alt' 😱
I'm having trouble finding documentation on what all the new keycodes are for Gutenberg. I know how to add a code tag, but how do I actually make a code block? I can make a heading just by using markdown. Same with bolding stuff or making an ul or ol but not sure how to get the code block? |
@jcklpe A Code block can be added either using the inserter, or by key sequence using three tick (`) characters, then Enter. Inline code fragments can be made by typing "`", the code fragment, then a closing "`". My understanding is that a user guide should become available shortly in the Gutenberg Handbook. |
@aduth Awesome! I tried doing ``` like on github but duh, should have tried just one. Thanks! |
You should be able to insert a code block by typing |
Description
See #71.
This PR adds the remaining shortcuts for editable:
access+d
for strikethrough (<del>
tag).access+c
for code.access+a
andmeta+k
to add a link.access+s
to remove a link.access
meansctrl+alt
on Mac andshift+alt
on Windows.meta
meanscmd
on Mac andctrl
on Windows.How Has This Been Tested?
access+d
and the text should be struck through. Press it again and the formatting should be removed.access+x
and the text should be code. Press it again and the formatting should be removed.access+a
and the pop up to add a link should appear focussed.meta+k
and the pop up to add a link should appear focussed.access+s
after putting the caret on a link. The link should be removed.Checklist: