-
Notifications
You must be signed in to change notification settings - Fork 38
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(react): add missing data-size & data-color props to Details #2930
base: next
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 8f54ec9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
Preview deployments for this pull request: Storybook - |
Coverage Report
File Coverage
|
DefaultProps & HTMLAttributes<HTMLDetailsElement>, | ||
{ | ||
/** | ||
* Specify which color palette to use. | ||
*/ | ||
'data-color'?: DefaultProps['data-color']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DefaultProps
comes with data-color
, do we need to specify it again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something with MergeRight
or how react-docgen parses types not working as intended?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only reason I'm repeating it, is to make the documentation make sense. The DefaultProps['data-color']
doc comment says (emphasis mine):
Sets a color palette which may be used by descendants. Does not affect this component. If left unspecified, the color is inherited from the nearest ancestor with data-color.
When I implemented the new color system I landed on this as the best default documentation, since it would be confusing for components that don't change color based on this prop to have "Specify which color palette to use". But if you have any ideas or suggestions on how to do this more clearly, I'm all ears 😊
However, looking at Details' implementation, both the type and doc comment should actually be different. It should be something like this:
/**
* Specify which color palette to use.
* If left unspecified, the color is inherited from the nearest ancestor with data-color.
*/
'data-color'?: 'subtle' | Color;
90d2d3e
to
8f54ec9
Compare
No description provided.