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

Conversation

@youknowriad
Copy link
Contributor

youknowriad commented Jan 3, 2020

Address review comments related to the keyboard shortcuts refactor:

#19320 (comment)
https://github.com/WordPress/gutenberg/pull/19318/files#r362599806

@youknowriad youknowriad requested a review from aduth Jan 3, 2020
@youknowriad youknowriad requested a review from talldan as a code owner Jan 3, 2020
@youknowriad youknowriad self-assigned this Jan 3, 2020
@youknowriad youknowriad force-pushed the update/shortcut-tweaks branch from 473c22d to 66623aa Jan 3, 2020
/**
* A block selection object.
*
* @typedef {Object} WPKeyboarShortcutConfig

This comment has been minimized.

Copy link
@aduth

aduth Jan 3, 2020

Member

Typo:

Suggested change
* @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.

This comment has been minimized.

Copy link
@aduth

aduth Jan 3, 2020

Member
Suggested change
* @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.

This comment has been minimized.

Copy link
@aduth

aduth Jan 3, 2020

Member

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.

Suggested change
* {{[string]:Function}} Shortcut Formatting Methods.
* {{[string]:Function}} Shortcut Formatting Methods.

This comment has been minimized.

Copy link
@aduth

aduth Jan 3, 2020

Member

we could use Object type combined with @property tags.

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).

- _representation_ `string`: Type of reprensentation. (display, raw, ariaLabel )
- _representation_ `null`: Type of representation (display, raw, ariaLabel).
Comment on lines -69 to +69

This comment has been minimized.

Copy link
@aduth

aduth Jan 3, 2020

Member

Related: #18045

* @property {string} character Character.
* @property {string} [modifier] Modifier.
* @property {string} character Character.
* @property {WPKeycodeModifier|undefined} modifier Modifier.
Comment on lines -6 to +9

This comment has been minimized.

Copy link
@aduth

aduth Jan 3, 2020

Member

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

@aduth
aduth approved these changes Jan 3, 2020
@youknowriad

This comment has been minimized.

Copy link
Contributor Author

youknowriad commented Jan 3, 2020

Thanks for the updates Andrew

@youknowriad youknowriad merged commit fd31f20 into master Jan 3, 2020
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@youknowriad youknowriad deleted the update/shortcut-tweaks branch Jan 3, 2020
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.