digdir / designsystemet

Designsystemet
https://designsystemet.no
MIT License
85 stars 38 forks source link

fix(Alert): changes to type #2759

Open elaffen opened 2 weeks ago

elaffen commented 2 weeks ago

Changed Alert's children type to ReactNode. Prop-list in Storybook indicated that children was of type 'string'. That is not correct. And it is somewhat confusing and not consistent that children in other components not are listed in Storybook's prop-list. Chip for example has children, but it's not listed in the prop-list.

changeset-bot[bot] commented 2 weeks ago

⚠️ No Changeset found

Latest commit: d27ce0b44b441d8753be44d4a477c35f29d1afe4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

mimarz commented 2 weeks ago

Good morning!

Thanks for the PR and bringing this to our attention :)

We extend/join the default prop types for the native element used in a component (HTMLAttributes<HTMLDivElement>) so the type is already ReactNode.

Its not shown in the props table because Storybook filters away native props, so the props table is more of a table of props that differ from the underlying element.

image

Here it looks like the default value used for the preview story is used as the prop type displayed in the props for some reason 🤔

I would like to investigate this closer as we need to we need to find a solution to the core problem here that works for all components and future components. Having to manually remember to override specific native props on components is cumbersome to maintain and prone for errors.

I have made a new bug issue to follow up this: 2760