DropdownMenu: refactor to TypeScript #50187
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
chad1008
force-pushed
the
refactor-dropdown-menu-ts
branch
from
948180a
to
d87ca36
Compare
chad1008
commented
Apr 28, 2023
chad1008
force-pushed
the
refactor-dropdown-menu-ts
branch
from
d87ca36
to
df2694d
Compare
chad1008
commented
Apr 28, 2023
|
Size Change: +1 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
chad1008
force-pushed
the
refactor-dropdown-menu-ts
branch
3 times, most recently
from
a2e6038
to
e994728
Compare
chad1008
commented
May 1, 2023
chad1008
commented
May 2, 2023
chad1008
force-pushed
the
refactor-dropdown-menu-ts
branch
from
4e95457
to
b3b794c
Compare
chad1008
commented
May 3, 2023
ciampo
reviewed
May 4, 2023
| { /* There is an issue here with TS not recognizing the | ||
| * `RenderProp` being passed. | ||
| * @ts-expect-error */ } | ||
| { ( toggleProps ) => ( |
There was a problem hiding this comment.
Hey @diegohaz 👋 We're refactoring this component to TypeScript, and we're getting an error around the type of children. We're passing a render prop, but for some reason it doesn't seem to work with the type that ToolbarItem from reakit is expecting. Do you have any ideas why this is happening?
Thanks 🙏
There was a problem hiding this comment.
What's the error? You might need to explicitly type the parameter (instead of relying on inference).
There was a problem hiding this comment.
This is the error:
Type '(toggleProps: ExtractHTMLAttributes<any>) => Element' is not assignable to type '((string | number | boolean | ReactElement<any, string | JSXElementConstructor<any>> | ReactFragment | ReactPortal) & (string | ... 5 more ... | RenderProp<...>)) | null | undefined'.
Type '(toggleProps: ExtractHTMLAttributes<any>) => Element' is not assignable to type 'string & RenderProp<ExtractHTMLAttributes<any>>'.
Type '(toggleProps: ExtractHTMLAttributes<any>) => Element' is not assignable to type 'string'.ts(2322)
What I don't understand is the initial string & ... in the expected type. Do you have any ideas as to why? It seems that our render prop would otherwise be matching correctly the RenderProp type
There was a problem hiding this comment.
(also, found this related comment about ToolbarItem's children types #50262 (comment) )
There was a problem hiding this comment.
Hey @diegohaz , did you have time to take a look at this?
There was a problem hiding this comment.
Looks like it's coming from https://github.com/ariakit/ariakit/blob/3966597f6a24f601f83d0816231486b8a293faee/packages/reakit-utils/src/types.ts#L63
But I also don't understand why there's a string & ... there. It should be possible to fix this in the ToolbarItem component by providing own types for the children prop instead of inheriting them directly from Reakit:
There was a problem hiding this comment.
Awesome, thanks for that @diegohaz. I'll put that on my list of things to follow up on!
There was a problem hiding this comment.
Thank you, @diegohaz . I'll circle back to you in case we understand where the string & ... comes from in the resulting type
ciampo
reviewed
May 9, 2023
|
Flaky tests detected in 9b920e0. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4928472878
|
There is very little reason to use an |
|
Thanks @andrewhayward |
|
@ciampo I've added the changes you and I discussed (related to this investigation) to allow Typing |
ciampo
approved these changes
May 16, 2023
There was a problem hiding this comment.
Left a couple of comments, but overall the changes LGTM 🚀
Feel free to merge once pending comments are addressed (as an exception, let's not consider https://github.com/WordPress/gutenberg/pull/50187/files#r1185450867 blocking for this PR, we can open a follow-up once Diego comes back with an answer)
We should also rebase (or merge trunk) since this PR has stayed open for a while
chad1008
force-pushed
the
refactor-dropdown-menu-ts
branch
from
b38f617
to
0f925c1
Compare
8 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.
coauthored with @ciampo, who did most of the heavy lifting 💪
What?
Refactor
DropdownMenucomponent to TypeScriptPart of #35744
Why?
The refactor to TypeScript has many benefits (auto-generated docs, static linting and error prevention, better IDE experience). See #35744 for more details
How?
Followed the steps in the TypeScript migration guide
Notes
One decision we had to make while typing the component's
togglePropsprop was which set of props to import fromButton. We opted forButtonAsButtonPropsbecause semantically we felt like abuttonmade more sense than ananchor. I looked at current implementations ofDropDownMenuand didn't see any passing anhrefto the toggle so should be fine with existing usages.In my accessibility searches it didn't look like using a
buttonfor dropdown menus was a problem but I'd love a more experienced opinion... @andrewhayward do you have any thoughts? 😄Testing Instructions