refactor: Destructure default props #525
Conversation
|
I'm confused. The issue referenced is to change |
|
Test summary
Run details
View run in Cypress Dashboard ➡️ This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard |
|
@mannycarrera4 This will be a problem with all components - not just |
anicholls
added
the
breaking-change
|
@anicholls Yep yep, just opened up as a draft to work my way through and document it. |
| /** | ||
| * Type to Pick props from an interface, making some required and leaving others as is | ||
| */ | ||
| export type PickRequired<T, Req extends keyof T, AsIs extends keyof T> = Required<Pick<T, Req>> & |
There was a problem hiding this comment.
@NicholasBoll Thoughts on using this to avoid setting the default twice? (it was needed a second time for styled components)
There was a problem hiding this comment.
I think I get it. The external interface says some props are optional and will be defaulted, but the Styled function doesn't know about any defaults and requires an override to tell Typescript the props aren't undefined.
Interesting problem. I think this is fine, but it is another interface that has to be learned to keep Typescript + Styled API happy.
It makes the css prop more interesting as it won't suffer from this problem.
| @@ -11,12 +11,12 @@ export interface ModalContentProps extends React.HTMLAttributes<HTMLDivElement> | |||
| * The padding of the Modal. Accepts `zero`, `s`, or `l`. | |||
| * @default PopupPadding.l | |||
There was a problem hiding this comment.
Still waiting on this one. Need to make sure we update the JSDoc and readme as well.
| public render() { | ||
| const { | ||
| icon = checkIcon as CanvasSystemIcon, // needed for TS2742 - https://github.com/microsoft/TypeScript/issues/29808 |
There was a problem hiding this comment.
Interesting... Thank you for providing a link to the issue!
anicholls
changed the title
| const {children, gutter, capWidth, ...elemProps} = this.props; | ||
| const { | ||
| gutter = canvas.spacing.xs, | ||
| spacing = spacingNumbers.xs, |
There was a problem hiding this comment.
This default will not be set in the renderChild function.
| backgroundColor = SidePanelBackgroundColor.White, | ||
| openNavigationLabel = 'open navigation', | ||
| closeNavigationLabel = 'close navigation', | ||
| openDirection = SidePanelOpenDirection.Left, |
There was a problem hiding this comment.
This default won't be picked up in the private toggleButtonDirection function below.
Summary
Fixes: #486
This PR touches a lot of our files, except our internal components like static states.
TODO