MenuItemsChoice: Refactor to TypeScript
#47180
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
flootr
added
[Type] Code Quality
|
Size Change: +9 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
flootr
force-pushed
the
refactor/menu-items-choice-to-typescript
branch
from
78431eb
to
62c3b47
Compare
mirka
reviewed
Jan 24, 2023
| /** | ||
| * Array of choices | ||
| */ | ||
| choices: readonly MenuItemChoice[]; |
There was a problem hiding this comment.
Interesting! I've never really considered using readonly like this because it's a given that a React component won't mutate its props, but I guess it adds another layer of protection. Just curious, have you ever encountered situations where a component was inadvertently mutating a prop?
cc @ciampo
There was a problem hiding this comment.
Never personally seen anything like it, off the top of my mind. But curiously enough, I encountered recently readonly in the QueryControls typescript refactor PR: #46721
There was a problem hiding this comment.
To be honest this is a copy paste. I'll let you decide whether that's something we want to keep.
I've definitely seen code which reassigns and/or mutates props but it's not exactly considered a good practice and is usually avoided. As you mentioned I'd consider readonly an additional layer of protection which warns you when you're trying to e.g. .push something to the choices array instead of creating a new array and spread the existing array.
There was a problem hiding this comment.
I mean, it certainly doesn't hurt! I looked into it and some people actually suggest doing readonly by default on all arrays 😆 I will keep it in mind as well.
| } | ||
| } } | ||
| onMouseEnter={ () => onHover( item.value ) } | ||
| onMouseLeave={ () => onHover() } |
There was a problem hiding this comment.
Any reason why this was changed from () => onHover( null )? This could be a breaking change for a consumer that was specifically doing null checks, so I think we should avoid the change if possible.
There was a problem hiding this comment.
I understand we need to keep the API to make sure we don't introduce a breaking change at this point. There's no particular reason other than me finding this kind of an odd API to return explicitly null in case no element is hovered. Reverted via 9baa7fb.
flootr
force-pushed
the
refactor/menu-items-choice-to-typescript
branch
from
62c3b47
to
9baa7fb
Compare
|
Flaky tests detected in eff0379. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4113137924
|
mirka
approved these changes
Jan 30, 2023
| /** | ||
| * Array of choices | ||
| */ | ||
| choices: readonly MenuItemChoice[]; |
There was a problem hiding this comment.
I mean, it certainly doesn't hurt! I looked into it and some people actually suggest doing readonly by default on all arrays 😆 I will keep it in mind as well.
flootr
force-pushed
the
refactor/menu-items-choice-to-typescript
branch
2 times, most recently
from
c727398
to
856a1ba
Compare
ciampo
reviewed
Feb 6, 2023
There was a problem hiding this comment.
Left a couple of minor comments about missing @default tags in the JSDoc descriptions.
I also noticed that this component doesn't list any of its props in the README, which means that we don't really have to amend that as part of this PR, if we want to speed up the TS refactor
flootr
force-pushed
the
refactor/menu-items-choice-to-typescript
branch
from
2683aa2
to
6574991
Compare
This is necessary so it's shown on the documentation page of the Storybook entry.
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
ciampo
force-pushed
the
refactor/menu-items-choice-to-typescript
branch
from
6574991
to
eff0379
Compare
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.
What?
Refactor
MenuItemsChoicecomponent to TypeScriptWhy?
Part of #35744
How?
.tsxand created atypes.tsfile with relevant typesTesting Instructions
Wit this PR we introduce a story for this component. Please verify it behaves as expected.