RangeControl: Convert stories to TypeScript #41444
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
aaronrobertshaw
added
[Type] Enhancement
|
Size Change: 0 B Total Size: 1.25 MB ℹ️ View Unchanged
|
ciampo
reviewed
May 31, 2022
There was a problem hiding this comment.
Thank you for working on this, Aaron! It's already looking great :)
I'll leave a few observations here, mostly related to the specific types and controls displayed in the auto-generated docs:
- the type that is being shown for
afterIconandbeforeIconisIconType<unknown>, which is not very useful documentation — although I'm not sure if we can do much about it, since the type is defined in theIconcomponent (this is a known limitation of the docgen employed by Storybook). Maybe we can add some text to the prop description text, referencing theIconcomponent for more info ? - the control being used for the
asprop is a dropdown containing all the possible HTML elements. This is interesting, because in other components the docgen was not able to determine this list, and was simply using the "Object" type — because of that, we previously had to override the control type to be a simple string of text for theasprop. It would be interesting to understand the reason for this different behaviour - as a side note, do we think that it makes sense for
RangeControlto be polymorphic (i.e. to accept theasprop)? Any other value thaninputkind of breaks the component. - the color-related props' type (
color,railColor,trackColor) is displayed asColor. Maybe we could add some text in the description to mention that the prop accepts any CSS color string? - There's no need to wrap the default
COLORS.ui.themevalue with backticks in the props JSDocs - The fact that the docgen can't "expand" compound types makes it so that the
RangeStep,RangeMarksandRenderTooltipContentCallbacktypes are displayed in the docs, giving little information about what the actual type for the given prop is. I know this is not very elegant, but we could look into removing the type alias and leaving the expanded versions in the prop types? - the control for the
typeprop is currently an "Object". Since that prop's value can only be"stepper"orundefined, we should switch to a more fitting control (e.g. checkbox / radio?)
Good point! I'd want to remove the polymorphism, but this is already a stable component 😭
Personally I'm fine with the current description, especially since the control right next to it is a standard color picker. If we are going to rewrite the descriptions though, a sentence structure like "CSS color string for ____" would probably work without being too wordy. |
I think that the |
aaronrobertshaw
force-pushed
the
refactor/range-control-to-typescript
branch
from
146713c
to
1f2a73b
Compare
aaronrobertshaw
force-pushed
the
refactor/range-control-stories-to-typescript
branch
from
0a25607
to
7e06827
Compare
I've omitted the |
aaronrobertshaw
force-pushed
the
refactor/range-control-stories-to-typescript
branch
from
7e06827
to
381c12c
Compare
|
Thanks for the reviews @ciampo, I've had some unexpected AFK but will merge these next week when I'm back on deck. |
aaronrobertshaw
force-pushed
the
refactor/range-control-to-typescript
branch
from
6a7bda9
to
a304615
Compare
aaronrobertshaw
force-pushed
the
refactor/range-control-stories-to-typescript
branch
from
381c12c
to
3dff4c6
Compare
aaronrobertshaw
force-pushed
the
refactor/range-control-to-typescript
branch
from
237923b
to
a1fa7d3
Compare
aaronrobertshaw
force-pushed
the
refactor/range-control-stories-to-typescript
branch
from
16aff8e
to
7cb9c6d
Compare
ciampo
reviewed
Jul 4, 2022
aaronrobertshaw
force-pushed
the
refactor/range-control-stories-to-typescript
branch
from
0d1826f
to
12b2863
Compare
aaronrobertshaw
force-pushed
the
refactor/range-control-to-typescript
branch
from
9c812cf
to
e2f920f
Compare
See: #40535 (comment) for more information on why we are ignoring this error. TL;DR we are looking to address some conflicts between input control types for their value props.
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
aaronrobertshaw
force-pushed
the
refactor/range-control-to-typescript
branch
from
a435f81
to
f19616e
Compare
aaronrobertshaw
force-pushed
the
refactor/range-control-stories-to-typescript
branch
from
2a0961e
to
38ec8f2
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.
Depends on:
Related:
What?
Converts
RangeControlstorybook examples to TypeScript and updates them to match the latest approach to component stories.This is based off #40535 in order to separate discussion on storybook examples from the main effort of typing the
RangeControl.Why?
It's another step closer to typing the components package and improving the Storybook as living documentation.
How?
Testing Instructions
npm run storybook:dev