Migrate ProgressBar to TypeScript #987
Conversation
🦋 Changeset detectedLatest commit: d41d361 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/primer/primer-components/mjey7llq5 |
src/ProgressBar.tsx
Outdated
| } & WidthProps & | ||
| SystemCommonProps & | ||
| SxProp | ||
| >` |
There was a problem hiding this comment.
What do you think about creating an interface at the top of the file for situations like this where it includes multiple lines? My hunch is that for more complex components like this one it can get a little tricky to visually parse. That seems to be what other applications at GH do, might make it easier to read when jumping between codebases. Curious what @smockle or @T-Hugs thinks
There was a problem hiding this comment.
Yeah, I agree. I'd love to come up with a naming convention for this kind of props type that's passed to styled-components but not exported and doesn't contain all the props accepted by the returned component. Maybe Styled[component-name]Props (e.g. StyledProgressContainerProps)?
There was a problem hiding this comment.
What do you think about creating an interface at the top of the file for situations like this where it includes multiple lines? My hunch is that for more complex components like this one it can get a little tricky to visually parse.
Great idea! Incidentally, organizing prop types this way helped with the namespace components issue described in #999 (comment)
| display: ${props => (props.inline ? 'inline-flex' : 'flex')}; | ||
| overflow: hidden; | ||
| background-color: ${get('colors.gray.2')}; | ||
| border-radius: ${get('radii.1')}; | ||
| height: ${props => sizeMap[props.barSize]}; | ||
| height: ${props => sizeMap[props.barSize || 'default']}; |
There was a problem hiding this comment.
Is this necessary if we're setting the default value in defaultProps?
There was a problem hiding this comment.
It shouldn't be necessary. But defaultProps + styled-components + TypeScript don't play well together in this case. TypeScript was complaining about props.barSize possibly being undefined. I'm open to other solutions :)
This PR migrates the
ProgressBarcomponent to TypeScript as part of the TypeScript refactor.