Skip to content
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

Address review comments related to the keyboard shortcuts refactor: #19395

Merged
merged 3 commits into from
Jan 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ _Parameters_

- _state_ `Object`: Global state.
- _name_ `string`: Shortcut name.
- _representation_ `string`: Type of reprensentation. (display, raw, ariaLabel )
- _representation_ `null`: Type of representation (display, raw, ariaLabel).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: #18045


_Returns_

Expand Down
2 changes: 1 addition & 1 deletion packages/compose/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ _Parameters_

- _shortcuts_ `(Array<string>|string)`: Keyboard Shortcuts.
- _callback_ `Function`: Shortcut callback.
- _options_ `Object`: Shortcut options.
- _options_ `WPKeyboardShortcutConfig`: Shortcut options.

<a name="useMediaQuery" href="#useMediaQuery">#</a> **useMediaQuery**

Expand Down
17 changes: 14 additions & 3 deletions packages/compose/src/hooks/use-keyboard-shortcut/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,17 @@ import { includes, castArray } from 'lodash';
*/
import { useEffect } from '@wordpress/element';

/**
* A block selection object.
*
* @typedef {Object} WPKeyboardShortcutConfig
*
* @property {boolean} [bindGlobal] Handle keyboard events anywhere including inside textarea/input fields.
* @property {string} [eventName] Event name used to trigger the handler, defaults to keydown.
* @property {boolean} [isDisabled] Disables the keyboard handler if the value is true.
* @property {Object} [target] React reference to the DOM element used to catch the keyboard event.
*/

/**
* Return true if platform is MacOS.
*
Expand All @@ -27,9 +38,9 @@ function isAppleOS( _window = window ) {
/**
* Attach a keyboard shortcut handler.
*
* @param {string[]|string} shortcuts Keyboard Shortcuts.
* @param {Function} callback Shortcut callback.
* @param {Object} options Shortcut options.
* @param {string[]|string} shortcuts Keyboard Shortcuts.
* @param {Function} callback Shortcut callback.
* @param {WPKeyboardShortcutConfig} options Shortcut options.
*/
function useKeyboardShortcut( shortcuts, callback, {
bindGlobal = false,
Expand Down
2 changes: 1 addition & 1 deletion packages/editor/src/components/post-title/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ class PostTitle extends Component {
*/
/* eslint-disable jsx-a11y/no-autofocus */
autoFocus={ document.body === document.activeElement && isCleanNewPost }
/* eslint-enable jsx-a11y/no-autofocus */
/* eslint-enable jsx-a11y/no-autofocus */
/>
</div>
{ isSelected && isPostTypeViewable && <PostPermalink /> }
Expand Down
16 changes: 9 additions & 7 deletions packages/keyboard-shortcuts/src/store/actions.js
Original file line number Diff line number Diff line change
@@ -1,22 +1,24 @@
/** @typedef {import('@wordpress/keycodes').WPKeycodeModifier} WPKeycodeModifier */

/**
* Keyboard key combination.
*
* @typedef {Object} WPShortcutKeyCombination
*
* @property {string} character Character.
* @property {string} [modifier] Modifier.
* @property {string} character Character.
* @property {WPKeycodeModifier|undefined} modifier Modifier.
Comment on lines -6 to +9
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was a bit unfortunate, I found. The previous [modifier] optional property syntax should be fine, but for some reason the TypeScript tooling does not pick up on it being an issue unless it was explicitly a union type with undefined, even though it detects it as optional.

It only (correctly) flags an error when trying to use a modifier method on WPKeycodeHandlerByModifier when it's typed using the undefined union:

undefined-union

*/

/**
* Configuration of a registered keyboard shortcut.
*
* @typedef {Object} WPShortcutConfig
*
* @property {string} name Shortcut name.
* @property {string} category Shortcut category.
* @property {string} description Shortcut description.
* @property {WPShortcutKeyCombination} keyCombination Shortcut key combination.
* @property {WPShortcutKeyCombination[]} [aliases] Shortcut aliases.
* @property {string} name Shortcut name.
* @property {string} category Shortcut category.
* @property {string} description Shortcut description.
* @property {WPShortcutKeyCombination} keyCombination Shortcut key combination.
* @property {WPShortcutKeyCombination[]} [aliases] Shortcut aliases.
*/

/**
Expand Down
37 changes: 24 additions & 13 deletions packages/keyboard-shortcuts/src/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,35 @@ import { displayShortcut, shortcutAriaLabel, rawShortcut } from '@wordpress/keyc

/** @typedef {import('./actions').WPShortcutKeyCombination} WPShortcutKeyCombination */

/** @typedef {import('@wordpress/keycodes').WPKeycodeHandlerByModifier} WPKeycodeHandlerByModifier */

/**
* Shared reference to an empty array for cases where it is important to avoid
* returning a new array reference on every invocation.
*
* @type {Array}
* @type {Array<any>}
*/
const EMPTY_ARRAY = [];

/**
* Shortcut formatting methods.
*
* @property {WPKeycodeHandlerByModifier} display Display formatting.
* @property {WPKeycodeHandlerByModifier} rawShortcut Raw shortcut formatting.
* @property {WPKeycodeHandlerByModifier} ariaLabel ARIA label formatting.
*/
const FORMATTING_METHODS = {
display: displayShortcut,
raw: rawShortcut,
ariaLabel: shortcutAriaLabel,
};

/**
* Returns a string representing the key combination.
*
* @param {WPShortcutKeyCombination} shortcut Key combination.
* @param {string} representation Type of reprensentation. (display, raw, ariaLabel )
* @param {?WPShortcutKeyCombination} shortcut Key combination.
* @param {keyof FORMATTING_METHODS} representation Type of representation
* (display, raw, ariaLabel).
*
* @return {string?} Shortcut representation.
*/
Expand All @@ -32,14 +48,8 @@ function getKeyCombinationRepresentation( shortcut, representation ) {
return null;
}

const formattingMethods = {
display: displayShortcut,
raw: rawShortcut,
ariaLabel: shortcutAriaLabel,
};

return shortcut.modifier ?
formattingMethods[ representation ][ shortcut.modifier ]( shortcut.character ) :
FORMATTING_METHODS[ representation ][ shortcut.modifier ]( shortcut.character ) :
shortcut.character;
}

Expand All @@ -58,9 +68,10 @@ export function getShortcutKeyCombination( state, name ) {
/**
* Returns a string representing the main key combination for a given shortcut name.
*
* @param {Object} state Global state.
* @param {string} name Shortcut name.
* @param {string} representation Type of reprensentation. (display, raw, ariaLabel )
* @param {Object} state Global state.
* @param {string} name Shortcut name.
* @param {keyof FORMATTING_METHODS} representation Type of representation
* (display, raw, ariaLabel).
*
* @return {string?} Shortcut representation.
*/
Expand Down
12 changes: 8 additions & 4 deletions packages/keycodes/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ E.g. displayShortcut.primary( 'm' ) will return '⌘M' on Mac.

_Type_

- `Object` Keyed map of functions to display shortcuts.
- `WPKeycodeHandlerByModifier` Keyed map of functions to display shortcuts.

<a name="displayShortcutList" href="#displayShortcutList">#</a> **displayShortcutList**

Expand All @@ -74,7 +74,7 @@ E.g displayShortcutList.primary( 'm' ) will return [ '⌘', 'M' ] on Mac.

_Type_

- `Object` keyed map of functions to shortcut sequences
- `WPKeycodeHandlerByModifier` Keyed map of functions to shortcut sequences.

<a name="DOWN" href="#DOWN">#</a> **DOWN**

Expand All @@ -101,7 +101,7 @@ signals pressing ⌘M.

_Type_

- `Object` Keyed map of functions to match events.
- `WPKeycodeHandlerByModifier` Keyed map of functions to match events.

<a name="LEFT" href="#LEFT">#</a> **LEFT**

Expand Down Expand Up @@ -131,7 +131,7 @@ These are intended for user with the KeyboardShortcuts component or TinyMCE.

_Type_

- `Object` Keyed map of functions to raw shortcuts.
- `WPKeycodeHandlerByModifier` Keyed map of functions to raw shortcuts.

<a name="RIGHT" href="#RIGHT">#</a> **RIGHT**

Expand All @@ -146,6 +146,10 @@ Keycode for SHIFT key.
An object that contains functions to return an aria label for a keyboard shortcut.
E.g. shortcutAriaLabel.primary( '.' ) will return 'Command + Period' on Mac.

_Type_

- `WPKeycodeHandlerByModifier` Keyed map of functions to shortcut ARIA labels.

<a name="SPACE" href="#SPACE">#</a> **SPACE**

Keycode for SPACE key.
Expand Down
24 changes: 20 additions & 4 deletions packages/keycodes/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,17 @@ import { __ } from '@wordpress/i18n';
*/
import { isAppleOS } from './platform';

/**
* @typedef {'primary'|'primaryShift'|'primaryAlt'|'secondary'|'access'|'ctrl'|'alt'|'ctrlShift'|'shift'|'shiftAlt'} WPKeycodeModifier
*/

/**
* An object of handler functions for each of the possible modifier
* combinations. A handler will return a value for a given key.
*
* @typedef {{[M in WPKeycodeModifier]:(key:string)=>any}} WPKeycodeHandlerByModifier
*/

/**
* Keycode for BACKSPACE key.
*/
Expand Down Expand Up @@ -118,7 +129,7 @@ export const modifiers = {
* E.g. rawShortcut.primary( 'm' ) will return 'meta+m' on Mac.
* These are intended for user with the KeyboardShortcuts component or TinyMCE.
*
* @type {Object} Keyed map of functions to raw shortcuts.
* @type {WPKeycodeHandlerByModifier} Keyed map of functions to raw shortcuts.
*/
export const rawShortcut = mapValues( modifiers, ( modifier ) => {
return ( character, _isApple = isAppleOS ) => {
Expand All @@ -130,7 +141,8 @@ export const rawShortcut = mapValues( modifiers, ( modifier ) => {
* Return an array of the parts of a keyboard shortcut chord for display
* E.g displayShortcutList.primary( 'm' ) will return [ '⌘', 'M' ] on Mac.
*
* @type {Object} keyed map of functions to shortcut sequences
* @type {WPKeycodeHandlerByModifier} Keyed map of functions to shortcut
* sequences.
*/
export const displayShortcutList = mapValues( modifiers, ( modifier ) => {
return ( character, _isApple = isAppleOS ) => {
Expand Down Expand Up @@ -161,7 +173,8 @@ export const displayShortcutList = mapValues( modifiers, ( modifier ) => {
* An object that contains functions to display shortcuts.
* E.g. displayShortcut.primary( 'm' ) will return '⌘M' on Mac.
*
* @type {Object} Keyed map of functions to display shortcuts.
* @type {WPKeycodeHandlerByModifier} Keyed map of functions to display
* shortcuts.
*/
export const displayShortcut = mapValues( displayShortcutList, ( shortcutList ) => {
return ( character, _isApple = isAppleOS ) => shortcutList( character, _isApple ).join( '' );
Expand All @@ -170,6 +183,9 @@ export const displayShortcut = mapValues( displayShortcutList, ( shortcutList )
/**
* An object that contains functions to return an aria label for a keyboard shortcut.
* E.g. shortcutAriaLabel.primary( '.' ) will return 'Command + Period' on Mac.
*
* @type {WPKeycodeHandlerByModifier} Keyed map of functions to shortcut ARIA
* labels.
*/
export const shortcutAriaLabel = mapValues( modifiers, ( modifier ) => {
return ( character, _isApple = isAppleOS ) => {
Expand Down Expand Up @@ -199,7 +215,7 @@ export const shortcutAriaLabel = mapValues( modifiers, ( modifier ) => {
* E.g. isKeyboardEvent.primary( event, 'm' ) will return true if the event
* signals pressing ⌘M.
*
* @type {Object} Keyed map of functions to match events.
* @type {WPKeycodeHandlerByModifier} Keyed map of functions to match events.
*/
export const isKeyboardEvent = mapValues( modifiers, ( getModifiers ) => {
return ( event, character, _isApple = isAppleOS ) => {
Expand Down