-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 shortcut tooltips for main toolbar #6605
Changes from 2 commits
5be0ff4
3fca8d0
56389d6
a161ebc
90c3e90
f042a70
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,14 +25,21 @@ class IconButton extends Component { | |
const classes = classnames( 'components-icon-button', className ); | ||
const tooltipText = tooltip || label; | ||
|
||
// Should show the tooltip if an explicit tooltip is passed | ||
// or if there's a label and the children are empty and the tooltip is not explicitely disabled | ||
const showTooltip = !! tooltip || | ||
// Should show the tooltip... | ||
const showTooltip = ( | ||
// if an explicit tooltip is passed or... | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find this style of commenting each conditional really useful, especially after working on a patch around https://github.com/WordPress/gutenberg/blob/master/editor/components/block-list/block.js#L425 👍🏻 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A nitpick is moving the "if" above might make more sense, eg: // Should show the tooltip if...
const showTooltip = (
// an explicit tooltip is passed or... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So move all "if"s? "Or if..."? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think moving the if to the top and ending each line with But what you have already makes sense, I just think the wording could be tidied up a bit. If my suggestion is less clear than I could be wrong and should be ignored 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd say it could be: // Should show the tooltip if...
const showTooltip = (
// an explicit tooltip is passed or...
tooltip ||
// there's a shortcut or...
shortcut ||
(
// there's a label and...
label &&
// the children are empty and...
( ! children || ( isArray( children ) && ! children.length ) ) &&
// the tooltip is not explicitely disabled.
false !== tooltip
)
); (Includes the lack of |
||
!! tooltip || | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. I copied the previous code. |
||
// if there's a shortcut or... | ||
!! shortcut || | ||
( | ||
label && | ||
// if there's a label and... | ||
!! label && | ||
// the children are empty and... | ||
( ! children || ( isArray( children ) && ! children.length ) ) && | ||
// the tooltip is not explicitely disabled. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "explicitely" should be "explicitly" 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ugh yes, I just copied from the previous comment. :) |
||
false !== tooltip | ||
); | ||
) | ||
); | ||
|
||
let element = ( | ||
<Button { ...additionalProps } aria-label={ label } className={ classes } focus={ focus }> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,13 @@ | ||
/** | ||
* Internal dependencies | ||
* WordPress dependencies | ||
*/ | ||
import { secondaryKeyCode, secondaryShortcut } from 'utils/keycodes'; | ||
import { keycodes } from '@wordpress/utils'; | ||
|
||
const { rawShortcut, displayShortcut } = keycodes; | ||
|
||
export default { | ||
toggleEditorMode: { | ||
value: secondaryKeyCode( 'm' ), | ||
label: secondaryShortcut( 'M' ), | ||
value: rawShortcut.secondary( 'm' ), | ||
label: displayShortcut.secondary( 'm' ), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, the consistency here is better 👍 |
||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,6 +8,12 @@ | |
* on Windows Control will usually come first. So don't provide your own | ||
* shortcut combos directly to keyboardShortcut(). | ||
*/ | ||
|
||
/** | ||
* External dependencies | ||
*/ | ||
import { get, mapValues } from 'lodash'; | ||
|
||
export const BACKSPACE = 8; | ||
export const TAB = 9; | ||
export const ENTER = 13; | ||
|
@@ -23,8 +29,8 @@ export const F10 = 121; | |
|
||
export const ALT = 'alt'; | ||
export const CTRL = 'ctrl'; | ||
export const PRIMARY = 'mod'; | ||
export const META = 'meta'; | ||
// Understood in both Mousetrap and TinyMCE. | ||
export const COMMAND = 'meta'; | ||
export const SHIFT = 'shift'; | ||
|
||
/** | ||
|
@@ -38,99 +44,49 @@ export function isMacOS( _window = window ) { | |
return _window.navigator.platform.indexOf( 'Mac' ) !== -1; | ||
} | ||
|
||
/** | ||
* Create a keyboard shortcut based on a string of modifiers + key(s). | ||
* | ||
* This function is not intended to be used directly by developers. | ||
* Instead, use primaryShortcut(), accessShortcut(), etc. | ||
* | ||
* @param {string} keys Modifier and keyboard keys, seperated by "+". | ||
* @param {Object} _isMacOS isMacOS function by default; used for DI testing. | ||
* | ||
* @return {string} The keyboard shortcut. | ||
*/ | ||
export function keyboardShortcut( keys, _isMacOS = isMacOS ) { | ||
const isMac = _isMacOS(); | ||
|
||
const alt = isMac ? '⌥option' : 'Alt'; | ||
const meta = isMac ? '⌃control' : '⊞'; | ||
const primary = isMac ? '⌘' : 'Ctrl'; | ||
const shift = isMac ? '⇧shift' : 'Shift'; | ||
|
||
const replacementKeyMap = { | ||
[ ALT ]: alt, | ||
[ META ]: meta, | ||
[ PRIMARY ]: primary, | ||
[ SHIFT ]: shift, | ||
}; | ||
|
||
return keys | ||
.replace( /\s/g, '' ) | ||
.split( '+' ) | ||
.map( ( key ) => { | ||
return replacementKeyMap.hasOwnProperty( key ) ? | ||
replacementKeyMap[ key ] : key; | ||
} ) | ||
.join( '+' ) | ||
// Because we use just the clover symbol for MacOS's "command" key, remove | ||
// the key join character ("+") between it and the final character if that | ||
// final character is alphanumeric. ⌘S looks nicer than ⌘+S. | ||
.replace( /⌘\+([a-zA-Z0-9])$/g, '⌘$1' ); | ||
} | ||
|
||
/** | ||
* Create an access key shortcut based on a single character. | ||
* | ||
* Access key combo is: | ||
* - Control+Alt on MacOS. | ||
* - Shift+Alt on Windows/everywhere else. | ||
* | ||
* @param {string} character The character for the access combination. | ||
* @param {Object} _isMacOS isMacOS function by default; used for DI testing. | ||
* | ||
* @return {string} The keyboard shortcut. | ||
*/ | ||
export function accessShortcut( character, _isMacOS = isMacOS ) { | ||
return keyboardShortcut( accessKeyCode( character.toUpperCase(), _isMacOS ), _isMacOS ); | ||
} | ||
|
||
export function accessKeyCode( character, _isMacOS = isMacOS ) { | ||
const keyCombo = _isMacOS() ? `${ META }+${ ALT }` : `${ SHIFT }+${ ALT }`; | ||
return `${ keyCombo }+${ character }`; | ||
} | ||
const modifiers = { | ||
primary: ( _isMac ) => _isMac() ? [ COMMAND ] : [ CTRL ], | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is much more immediately understandable than my old way. ✨ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do these need to be run as functions? Will they ever change over the lifetime of the app? Edit: And if the answer is for testing, it shouldn't really impact the implementation so much. Can be "good" for it to be made generic, though ideally we have some caching along the way in runtime use, so we're not running There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's all for testing; previously this was run once and stored in a constant, but it was also entirely untested. It could probably be cached somehow as a perf-versus-readability tradeoff. 😄 It shouldn't change in regular usage though, so yeah it's worth doing. |
||
primaryShift: ( _isMac ) => _isMac() ? [ SHIFT, COMMAND ] : [ CTRL, SHIFT ], | ||
secondary: ( _isMac ) => _isMac() ? [ SHIFT, ALT, COMMAND ] : [ CTRL, SHIFT, ALT ], | ||
access: ( _isMac ) => _isMac() ? [ CTRL, ALT ] : [ SHIFT, ALT ], | ||
}; | ||
|
||
/** | ||
* Create a modifier shortcut based on a single character. | ||
* | ||
* This will output Ctrl+G on Windows when "G" is supplied as an argument. | ||
* This will output Command+G on MacOS when "G" is supplied as an argument. | ||
* An object that contains functions to get raw shortcuts. | ||
* E.g. rawShortcut.primary( 'm' ) will return 'meta+m' on Mac. | ||
* These are intended for user with the KeyboardShortcuts component or TinyMCE. | ||
* | ||
* @param {string} character The character for the key command. | ||
* @param {Object} _isMacOS isMacOS function by default; used for DI testing. | ||
* | ||
* @return {string} The keyboard shortcut. | ||
* @type {Object} Keyed map of functions to raw shortcuts. | ||
*/ | ||
export function primaryShortcut( character, _isMacOS = isMacOS ) { | ||
return keyboardShortcut( `${ PRIMARY }+${ character.toUpperCase() }`, _isMacOS ); | ||
} | ||
export const rawShortcut = mapValues( modifiers, ( modifier ) => { | ||
return ( character, _isMac = isMacOS ) => { | ||
return [ ...modifier( _isMac ), character.toLowerCase() ].join( '+' ); | ||
}; | ||
} ); | ||
|
||
/** | ||
* Create an access key + primary key shortcut based on a single character. | ||
* | ||
* Access key combo is: | ||
* - Control+Alt+Command on MacOS. | ||
* - Control+Shift+Alt on Windows/everywhere else. | ||
* | ||
* @param {string} character The character for the access combination. | ||
* @param {Object} _isMacOS isMacOS function by default; used for DI testing. | ||
* An object that contains functions to display shortcuts. | ||
* E.g. displayShortcut.primary( 'm' ) will return '⌘M' on Mac. | ||
* | ||
* @return {string} The keyboard shortcut. | ||
* @type {Object} Keyed map of functions to display shortcuts. | ||
*/ | ||
export function secondaryShortcut( character, _isMacOS = isMacOS ) { | ||
return keyboardShortcut( secondaryKeyCode( character.toUpperCase(), _isMacOS ), _isMacOS ); | ||
} | ||
export const displayShortcut = mapValues( modifiers, ( modifier ) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, cool. Sorry I didn't think to use this, I'm not used to having lodash around. |
||
return ( character, _isMac = isMacOS ) => { | ||
const isMac = _isMac(); | ||
const replacementKeyMap = { | ||
[ ALT ]: isMac ? '⌥option' : 'Alt', | ||
[ CTRL ]: isMac ? '⌃control' : 'Ctrl', | ||
[ COMMAND ]: '⌘', | ||
[ SHIFT ]: isMac ? '⇧shift' : 'Shift', | ||
}; | ||
const shortcut = [ | ||
...modifier( _isMac ).map( ( key ) => get( replacementKeyMap, key, key ) ), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You defeated my new ESLint custom rule! (#6247) Which is to say, I should make this rule more strict 😛 |
||
character.toUpperCase(), | ||
].join( '+' ); | ||
|
||
export function secondaryKeyCode( character, _isMacOS = isMacOS ) { | ||
const keyCombo = _isMacOS() ? `${ SHIFT }+${ ALT }+${ PRIMARY }` : `${ PRIMARY }+${ SHIFT }+${ ALT }`; | ||
return `${ keyCombo }+${ character }`; | ||
} | ||
// Because we use just the clover symbol for MacOS's "command" key, remove | ||
// the key join character ("+") between it and the final character if that | ||
// final character is alphanumeric. ⌘S looks nicer than ⌘+S. | ||
return shortcut.replace( /⌘\+([A-Z0-9])$/g, '⌘$1' ); | ||
}; | ||
} ); |
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.
Yay, when I saw these I thought "this could be refactored". This is great 👍