BoxControl: Update story and refactor to Typescript
#56462
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
brookewp
added
[Type] Developer Documentation
|
Size Change: +1.02 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Nov 23, 2023
ciampo
reviewed
Nov 23, 2023
| const meta: Meta< typeof BoxControl > = { | ||
| title: 'Components (Experimental)/BoxControl', | ||
| component: BoxControl, | ||
| argTypes: { |
There was a problem hiding this comment.
Since the example Template below controls the BoxControl by passing it the value prop, we should disable controls for the value prop
There was a problem hiding this comment.
The controls for the values prop are still enabled
I believe that the right setting is values: { control: { type: null } }
ciampo
reviewed
Nov 23, 2023
ciampo
reviewed
Nov 23, 2023
|
|
||
| const Template: StoryFn< typeof BoxControl > = ( props ) => { | ||
| const [ values, setValues ] = | ||
| useState< BoxControlValue >( defaultSideValues ); |
There was a problem hiding this comment.
No need to use BoxControlValue, we can use directly
Suggested change
| useState< BoxControlValue >( defaultSideValues ); | |
| useState< ( typeof props )[ 'values' ] >( defaultSideValues ); |
There was a problem hiding this comment.
Interesting! I tried (typeof BoxControl)[ 'values' ] without success. Not sure I understand why that didn't work as well 🤔
There was a problem hiding this comment.
typeof BoxControl would return the component type (I believe it would tell you if the component is a class component, a function component, a JSX intrinsic string...), but that type doesn't have the prop types as properties.
On the other hand, typeof props returns the computed type for the properties of the BoxControl component — and that type has the value property.
There was a problem hiding this comment.
Expanding on this, an alternative version is also to grab the type of the props by using the React.ComponentProps utility type, which requires as argument the component type :
React.ComponentProps< typeof BoxControl >[ 'values' ]
I'm ok with keeping ( typeof props )[ 'values' ], but I hope this helps with understanding these types!
ciampo
reviewed
Nov 23, 2023
ciampo
reviewed
Nov 23, 2023
ciampo
reviewed
Nov 24, 2023
Comment on lines
42
to
43
| const [ values, setValues ] = | ||
| useState< ( typeof props )[ 'values' ] >( defaultSideValues ); |
There was a problem hiding this comment.
While testing that the onChange callback was being shown correctly in the actions tab in Storybook, I realised that the values being reported looked incorrect.
For example, interacting with BoxControl in the "Single Side" example should result in only changing the bottom value, but with the changes from this PR, the resulting value includes values also for the remaining sides:
After looking into this, I realised that this is happening because we're initializing the component always with the same defaultValues object, which includes values for all sides. It seems that BoxControl only updates the side that gets updated, and leaves the remaining sides untouched.
Prior to these changes, each example had its own specific default value, which prevented this issue from manifesting.
An easy fix for now is to avoid passing any default values
Suggested change
| const [ values, setValues ] = | |
| useState< ( typeof props )[ 'values' ] >( defaultSideValues ); | |
| const [ values, setValues ] = | |
| const [ values, setValues ] = useState< ( typeof props )[ 'values' ] >(); |
Definitely not in the scope of this PR, although I wonder if the component should somehow "validate" the incoming value, and compare it against the sides prop to see if there is any mismatch? Probably something with very low-priority, though, since it doesn't look like this component is being used much across the editor.
ciampo
approved these changes
Nov 24, 2023
There was a problem hiding this comment.
Preemptively approving, since this PR should be good to be merged after remaining open feedback (#56462 (comment), #56462 (comment)) is addressed 🚀
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?
Via #56185 (comment) it was found that
BoxControl's story hasn't been updated to use new storybook features. This required updating to Typescript as well.Why?
For consistency and easier use of the docs.
How?
Refactoring to Typescript, following our storybook standards by adding a meta object, and using the template and args format.
Testing Instructions
npm run: storybook devBoxControlexamples behave and look the same as before