ToolsPanel component: refactor to typescript #34028
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glendaviesnz
added
the
[Status] In Progress
|
Size Change: +79 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
ramonjd
reviewed
Aug 13, 2021
ciampo
added
[Feature] Component System
7 tasks
|
Just an FYI that the A rebase + refactor of all types beginning with |
Thanks for letting me know @ciampo |
glendaviesnz
force-pushed
the
refactor/tools-panel-to-typescript
branch
from
16f6def to
9f13cfa
Compare
|
Hey @glendaviesnz , for the sake of coordinating/tracking work related to components and global styles sidebar, do you have an estimate of when this PR is going to be ready for review? |
@ciampo, this was waiting on some refactors of this component that @aaronrobertshaw has on the go - we were planning to take the hit on the conflicts on this PR, rather than complicate those other PRs by merging this first. Aaron, do you have any thoughts on timelines for the other ToolsPanel work that is underway? |
Thank you for the explanation! I guess you're referring to #34530 ? I understand that you'd rather handle conflicts in this PR than slowing down other open PRs — I'd just like to make sure that this PR doesn't get too de-prioritised, since it looks like the |
I believe the primary PR is #34157 with #34530 secondary to that. It looks like we might be able to merge #34157 and handle passing the tools panel context through the slot's It's less clear how long #34530 might take. This one is less critical so if needed it could be redone after the TypeScript conversion.
The TypeScript conversion is definitely a priority and the need to make that happen has been discussed a few times in recent days. At this point, the path of least resistance still appears to be as @glendaviesnz suggested, handling the conflicts in this PR and completing the conversion prior to any further tweaks. For some additional background, the reason for #34157 being done before the conversion was to attempt to resolve an issue with duplicate "Dimensions" panels for the Cover block in the last release. |
|
Thank you for the explanation, @aaronrobertshaw! I'll keep my eyes open for future progress on this PR once #34157 (and follow-ups) get merged! |
glendaviesnz
force-pushed
the
refactor/tools-panel-to-typescript
branch
from
9f13cfa to
2052c1b
Compare
There was a problem hiding this comment.
Nice work here @glendaviesnz! I just left a couple of comments about the DropDownMenu props types — I know this is is a WIP, and my TypeScript skills a pretty rusty, so apologies if the comments aren't relevant or if you were in the middle of working on it 🙂
glendaviesnz
changed the title
glendaviesnz
removed
the
[Status] In Progress
|
@ciampo this is ready for some review now. This is the first bit of typescript I have done in quite some time, so very open to feedback on this. |
ciampo
reviewed
Sep 16, 2021
There was a problem hiding this comment.
Hey @glendaviesnz , thank you for working on this PR!
I've noticed that this PR includes changes to four components: DropdownMenu, MenuGroup, MenuItem and ToolsPanel. I believe we should have a separate PR for each of these components, which will the review process a lot easier and more focused:
- I propose we keep this PR focused on
ToolsPanel— if you get any TypeScript error when importing dependencies into theToolsPanel's TypeScript files, you can add// @ts-nocheckat the start of the imported file to temporarily suppress those warning (see an example of this for thePopovercomponent) - We can open follow-up PRs to take care of typing
MenuGroupandMenuItem. Typing theDropdownMenuextensively can be complicated, given how many of its props reference other components' props (popoverProps,toggleProps,menuProps) and how the arguments of thechildrenprop depend on theDropdown'srenderContentprop — we should skip this for now
Apart from that, I noticed that some of the files in the tools-panel folder are still with the .js extension — ideally, we'd want to convert all JS files to .ts(x) (with the only exception of stories/index.js).
The DropdownMenu, MenuGroup and MenuItem doc block type annotations have been removed from this PR so they can be handled separately as suggested.
I've switched the remaining ToolsPanel files from After making these changes:
@ciampo Would it be best to create new PRs for MenuGroup/MenuItem etc using the doc block annotations removed from this PR as their basis? So that those can be merged quickly, or should the new PRs aim to complete a full conversion to TypeScript for those components? |
ciampo
reviewed
Sep 17, 2021
There was a problem hiding this comment.
I've switched the remaining ToolsPanel files from
jstots
Thank you!
the only lint errors were those relating to DropdownMenu, MenuGroup and MenuItem as expected after rolling back those changes.
After addingts-nocheckthere are no remaining linting errors.
My bad, I haven't explained myself clearly enough. The // @ts-nockeck comment shouldn't be added to ToolsPanel — it should be added, instead, to all of the dependencies that ToolsPanel pulls in (DropdownMenu, MenuGroup, MenuItem, and recursively all of their local dependencies).
These are the steps that I would follow:
- Remove the
// @ts-nocheckcomment fromToolsPanelHeader - Add the
src/tools-panel/**/*folder totsconfig.json. This step is actually necessary to add the component to the TypeScript project, and thus enable TypeScript checks (so far they were not! My bad for not noticing this until now) - Run
npm run devand find all the errors that refer to files outside of thesrc/tools-panel/folder. We should add// @ts-nocheckat the start of each of those files, and add their folders totsconfig.json(similarly to what done in step 2). In our case, the folders should besrc/dropdown/**/*,src/dropdown-menu/**/*,src/menu-item/**/*,src/menu-group/**/*andsrc/navigable-container/**/*. - At this point, the
MenuGroup,MenuItemandDropdownMenucomponents are still showing errors insideToolsPanelHeader. To solve them, we need to slightly refactor these components and move thepropsdestructuring away from the function signature — this is to prevent TypeScript from inferring the types of these props. For the same reason, we will also need to temporarily delete the JSDoc types ofMenuItem(but we will add those back soon enough when converting it to TypeScript). (thank you @sarayourfriend for helping with this step!)
To speed things up, I've prepared a diff with all the changes that would result from the above steps
Click to expand
diff --git a/packages/components/src/dropdown-menu/index.js b/packages/components/src/dropdown-menu/index.js
index f2ae27cd91..e347007923 100644
--- a/packages/components/src/dropdown-menu/index.js
+++ b/packages/components/src/dropdown-menu/index.js
@@ -34,22 +34,24 @@ function mergeProps( defaultProps = {}, props = {} ) {
return mergedProps;
}
-function DropdownMenu( {
- children,
- className,
- controls,
- icon = menu,
- label,
- popoverProps,
- toggleProps,
- menuProps,
- disableOpenOnArrowDown = false,
- text,
- // The following props exist for backward compatibility.
- menuLabel,
- position,
- noIcons,
-} ) {
+function DropdownMenu( props ) {
+ const {
+ children,
+ className,
+ controls,
+ icon = menu,
+ label,
+ popoverProps,
+ toggleProps,
+ menuProps,
+ disableOpenOnArrowDown = false,
+ text,
+ // The following props exist for backward compatibility.
+ menuLabel,
+ position,
+ noIcons,
+ } = props;
+
if ( menuLabel ) {
deprecated( '`menuLabel` prop in `DropdownComponent`', {
since: '5.3',
diff --git a/packages/components/src/dropdown/index.js b/packages/components/src/dropdown/index.js
index ebee9c2b5a..09477a87e0 100644
--- a/packages/components/src/dropdown/index.js
+++ b/packages/components/src/dropdown/index.js
@@ -1,3 +1,4 @@
+// @ts-nocheck
/**
* External dependencies
*/
diff --git a/packages/components/src/menu-group/index.js b/packages/components/src/menu-group/index.js
index 19e182ddc7..6054bfcb3e 100644
--- a/packages/components/src/menu-group/index.js
+++ b/packages/components/src/menu-group/index.js
@@ -1,3 +1,4 @@
+// @ts-nocheck
/**
* External dependencies
*/
@@ -9,12 +10,8 @@ import classnames from 'classnames';
import { Children } from '@wordpress/element';
import { useInstanceId } from '@wordpress/compose';
-export function MenuGroup( {
- children,
- className = '',
- label,
- hideSeparator,
-} ) {
+export function MenuGroup( props ) {
+ const { children, className = '', label, hideSeparator } = props;
const instanceId = useInstanceId( MenuGroup );
if ( ! Children.count( children ) ) {
diff --git a/packages/components/src/menu-item/index.js b/packages/components/src/menu-item/index.js
index 8bed60d84d..f8a97b801b 100644
--- a/packages/components/src/menu-item/index.js
+++ b/packages/components/src/menu-item/index.js
@@ -1,3 +1,4 @@
+// @ts-nocheck
/**
* External dependencies
*/
@@ -16,23 +17,8 @@ import Shortcut from '../shortcut';
import Button from '../button';
import Icon from '../icon';
-/**
- * Renders a generic menu item for use inside the more menu.
- *
- * @param {Object} props Component props.
- * @param {WPElement} props.children Element to render as child of button.
- * @param {string} props.info Text to use as description for button text.
- * @param {string} props.className Class to set on the container.
- * @param {WPIcon} props.icon Button's `icon` prop.
- * @param {string|Object} props.shortcut Shortcut's `shortcut` prop.
- * @param {boolean} props.isSelected Whether or not the menu item is currently selected.
- * @param {string} [props.role="menuitem"] ARIA role of the menu item.
- * @param {Object} ref React Element ref.
- *
- * @return {WPComponent} The component to be rendered.
- */
-export function MenuItem(
- {
+export function MenuItem( props, ref ) {
+ let {
children,
info,
className,
@@ -40,10 +26,9 @@ export function MenuItem(
shortcut,
isSelected,
role = 'menuitem',
- ...props
- },
- ref
-) {
+ ...buttonProps
+ } = props;
+
className = classnames( 'components-menu-item__button', className );
if ( info ) {
@@ -72,7 +57,7 @@ export function MenuItem(
}
role={ role }
className={ className }
- { ...props }
+ { ...buttonProps }
>
<span className="components-menu-item__item">{ children }</span>
<Shortcut
diff --git a/packages/components/src/navigable-container/container.js b/packages/components/src/navigable-container/container.js
index 68734f89be..360bcb21b9 100644
--- a/packages/components/src/navigable-container/container.js
+++ b/packages/components/src/navigable-container/container.js
@@ -1,3 +1,4 @@
+// @ts-nocheck
/**
* External dependencies
*/
diff --git a/packages/components/src/navigable-container/index.js b/packages/components/src/navigable-container/index.js
index e98f1b1236..b47667fd39 100644
--- a/packages/components/src/navigable-container/index.js
+++ b/packages/components/src/navigable-container/index.js
@@ -1,3 +1,4 @@
+// @ts-nocheck
/**
* Internal Dependencies
*/
diff --git a/packages/components/src/navigable-container/menu.js b/packages/components/src/navigable-container/menu.js
index 6e0dc794d0..df238853df 100644
--- a/packages/components/src/navigable-container/menu.js
+++ b/packages/components/src/navigable-container/menu.js
@@ -1,3 +1,4 @@
+// @ts-nocheck
/**
* External dependencies
*/
diff --git a/packages/components/src/navigable-container/tabbable.js b/packages/components/src/navigable-container/tabbable.js
index bc572522e9..00ba8117b8 100644
--- a/packages/components/src/navigable-container/tabbable.js
+++ b/packages/components/src/navigable-container/tabbable.js
@@ -1,3 +1,4 @@
+// @ts-nocheck
/**
* WordPress dependencies
*/
diff --git a/packages/components/src/tools-panel/tools-panel-header/component.tsx b/packages/components/src/tools-panel/tools-panel-header/component.tsx
index 2ef7ab290c..50f6b1564a 100644
--- a/packages/components/src/tools-panel/tools-panel-header/component.tsx
+++ b/packages/components/src/tools-panel/tools-panel-header/component.tsx
@@ -1,7 +1,3 @@
-// This can be removed once typing has been completed for DropdownMenu,
-// MenuGroup & MenuItem.
-// @ts-nocheck
-
/**
* External dependencies
*/
diff --git a/packages/components/tsconfig.json b/packages/components/tsconfig.json
index a0a0e879a3..cd26b356e6 100644
--- a/packages/components/tsconfig.json
+++ b/packages/components/tsconfig.json
@@ -31,6 +31,8 @@
"src/disabled/**/*",
"src/divider/**/*",
"src/draggable/**/*",
+ "src/dropdown/**/*",
+ "src/dropdown-menu/**/*",
"src/elevation/**/*",
"src/flex/**/*",
"src/flyout/**/*",
@@ -41,6 +43,9 @@
"src/item-group/**/*",
"src/input-control/**/*",
"src/icon/**/*",
+ "src/menu-item/**/*",
+ "src/menu-group/**/*",
+ "src/navigable-container/**/*",
"src/number-control/**/*",
"src/popover/**/*",
"src/range-control/**/*",
@@ -55,6 +60,7 @@
"src/text/**/*",
"src/tip/**/*",
"src/toggle-group-control/**/*",
+ "src/tools-panel/**/*",
"src/tooltip/**/*",
"src/truncate/**/*",
"src/ui/**/*",Now that the issues related to dependencies should be solved, and since we've added the src/tools-panel/**/* folder to tsconfig.json, TypeScript should flag all of the remaining errors on the ToolsPanel set of components that still need addressing.
Would it be best to create new PRs for MenuGroup/MenuItem etc using the doc block annotations removed from this PR as their basis? So that those can be merged quickly, or should the new PRs aim to complete a full conversion to TypeScript for those components?
I would prefer if we could achieve a full conversion of both MenuGroup and MenuItem to TypeScript in two separate PRs — both components are quite small and the conversion shouldn't be too complicated. You can definitely use the annotations that were part of this PR as a starting point! I would also recommend focusing on this PR first and opening those 2 PRs as follow-ups.
7 tasks
|
Thanks for the detailed feedback, explanation and diff @ciampo! It helped me a lot. I've applied the diff and had a go (with the assistance of @glendaviesnz) at resolving all the typing errors that were exposed. I expect that there will probably be better approaches in some cases than what I've taken. Let me know what needs improving and I'll make it happen 🙂 As it stands now, after these updates, the ToolsPanel is functioning correctly for me in the storybook, editor and unit tests. |
This reverts commit 2676061.
This reverts commit 81c0048.
The typing for DropdownMenu, MenuGroup and MenuItem will be handled separately and may entail a full TypeScript conversion rather than only doc block annotations.
Removes redundant typing, improves naming and adds JSDocs etc
In order to prevent excessive renders and guard against consumers not memoizing the hasValue and resetAllFilter functions, these changes include useCallback calls to handle this before adding them to the hook dependencies.
aaronrobertshaw
force-pushed
the
refactor/tools-panel-to-typescript
branch
from
1be2e7b to
1f833ab
Compare
7 tasks
36 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Converts the ToolsPanel component to typescript. Also adds type annotations to the Menu components that it uses.
How has this been tested?