Address review comments related to the keyboard shortcuts refactor: #19395
Conversation
youknowriad added the [Type] Code Quality 
youknowriad force-pushed the update/shortcut-tweaks branch from 473c22d to 66623aa Compare
youknowriad requested review from ajitbohra and jorgefilipecosta as code owners
| /** | ||
| * A block selection object. | ||
| * | ||
| * @typedef {Object} WPKeyboarShortcutConfig |
There was a problem hiding this comment.
Typo:
| * @typedef {Object} WPKeyboarShortcutConfig | |
| * @typedef {Object} WPKeyboardShortcutConfig |
| * @param {Object} options Shortcut options. | ||
| * @param {string[]|string} shortcuts Keyboard Shortcuts. | ||
| * @param {Function} callback Shortcut callback. | ||
| * @param {WPKeyboarShortcutConfig} options Shortcut options. |
There was a problem hiding this comment.
| * @param {WPKeyboarShortcutConfig} options Shortcut options. | |
| * @param {WPKeyboardShortcutConfig} options Shortcut options. |
| @@ -19,6 +19,15 @@ import { displayShortcut, shortcutAriaLabel, rawShortcut } from '@wordpress/keyc | |||
| */ | |||
| const EMPTY_ARRAY = []; | |||
| /** | |||
| * {{[string]:Function}} Shortcut Formatting Methods. | |||
There was a problem hiding this comment.
This is probably one where, since the keys are known, we could use Object type combined with @property tags.
In any case, since this isn't using @type, it's not doing anything currently anyways.
Finally, it's actually an object two levels deep before getting to the function, since each of the "handlers" (displayShortcut, etc) are actually objects themselves.
| * {{[string]:Function}} Shortcut Formatting Methods. | |
| * {{[string]:Function}} Shortcut Formatting Methods. |
There was a problem hiding this comment.
we could use
Objecttype combined with@propertytags.
In implementing this, I've found that using the {Object} type in TypeScript has a high likelihood to trigger the any type fallback unintentionally, even when complemented by @property tags or types for the keys (see also this syntax vs. Object<M in WPKeycodeModifier,Function>).
We probably need to work through this a bit more. Not sure if it's a shortcoming of TypeScript, or just an accurate reflection of the "Object" type allowing for more keys than those explicitly defined (e.g. prototype members).
youknowriad requested review from chrisvanpatten, ellatrix and mkaz as code owners
| - _representation_ `string`: Type of reprensentation. (display, raw, ariaLabel ) | ||
| - _representation_ `null`: Type of representation (display, raw, ariaLabel). |
| * @property {string} character Character. | ||
| * @property {string} [modifier] Modifier. | ||
| * @property {string} character Character. | ||
| * @property {WPKeycodeModifier|undefined} modifier Modifier. |
There was a problem hiding this comment.
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:
Thanks for the updates Andrew |
Address review comments related to the keyboard shortcuts refactor:
#19320 (comment)
https://github.com/WordPress/gutenberg/pull/19318/files#r362599806