TypeScript: migrate keyboard-shortcuts package to TS#70898
TypeScript: migrate keyboard-shortcuts package to TS#70898kushagra-goyal-14 wants to merge 4 commits intoWordPress:trunkfrom
Conversation
kushagra-goyal-14
changed the title
kushagra-goyal-14
requested review from
ajitbohra,
ellatrix,
fabiankaegy,
juanmaguitar and
ndiego
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
t-hamano
added
[Type] Code Quality
| const useShortcut = (): null => null; | ||
| export default useShortcut; |
There was a problem hiding this comment.
We don't need to set the return type here. TS can inference it easily and it will preserve the git history for the file.
|
Updated the changes 🙌 |
manzoorwanijk
left a comment
manzoorwanijk
left a comment
There was a problem hiding this comment.
This looks good but I have some suggestions
| /** | ||
| * External dependencies | ||
| */ | ||
| import type { JSX } from 'react'; |
There was a problem hiding this comment.
Can we please directly use React.JSX without this import here like we have used React.HTMLAttributes below?
| * @property {string} character Character. | ||
| * @property {WPKeycodeModifier|undefined} modifier Modifier. | ||
| */ | ||
| export interface WPShortcutKeyCombination { |
There was a problem hiding this comment.
Let us remove the WP prefix from the type here.
| * @property {WPShortcutKeyCombination} keyCombination Shortcut key combination. | ||
| * @property {WPShortcutKeyCombination[]} [aliases] Shortcut aliases. | ||
| */ | ||
| export interface WPShortcutConfig { |
There was a problem hiding this comment.
Let us remove the WP prefix from the type here as well.
| interface ShortcutState { | ||
| category: string; | ||
| keyCombination: WPShortcutKeyCombination; | ||
| aliases?: WPShortcutKeyCombination[]; | ||
| description: string; | ||
| } | ||
|
|
||
| type ShortcutsState = Record< string, ShortcutState >; |
There was a problem hiding this comment.
Can we please move these types to a types.ts file? That means we will have less changes to the file and its git history will be preserved.
|
Any updates on this? |
3 participants
What?
Part of #67691
Migrating the keyboard-shortcuts package to TypeScript.
Why?
To enhance DX and add type safety.
How?
By porting the code and tests to TypeScript.
Testing Instructions
Typecheck and unit tests.