Skip to content
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

feat: add storybook #854

Merged
merged 9 commits into from
Sep 10, 2024
Merged

feat: add storybook #854

merged 9 commits into from
Sep 10, 2024

Conversation

ddecrulle
Copy link
Contributor

No description provided.

@ddecrulle ddecrulle requested a review from garronej September 10, 2024 11:28
@garronej
Copy link
Contributor

Thank you for the PR!

Please fix this it's an actual bug, not a matter of coding style and consistency. A render function (a function that returns a JSX Element) isn't equivelent to a react component.

Please fix the build as well.

Next time be sure to only tag me for review when everything is green.

Looking good non the less.

Copy link

@ddecrulle
Copy link
Contributor Author

Thank you for the PR!

Please fix this it's an actual bug, not a matter of coding style and consistency. A render function (a function that returns a JSX Element) isn't equivelent to a react component.

Please fix the build as well.

Next time be sure to only tag me for review when everything is green.

Looking good non the less.

This is not really a bug as you can see here : storybookjs/storybook#21115

By the way the hooks is not really necessary so I removed.

@garronej
Copy link
Contributor

garronej commented Sep 10, 2024

This is not really a bug as you can see here : storybookjs/storybook#21115

It's a bug.
When an API expect a function render or named otherwise but without upper case there's no implicit guarenty that this function will be used as a React component.
Maybe it is in this specific instance but it's generally not the case.

This for example works as expected. We're not breaking any rule:
https://stackblitz.com/edit/vitejs-vite-qvvrir?file=src%2FApp.tsx

This on the other end dosen't:
https://stackblitz.com/edit/vitejs-vite-hij9ml?file=src%2FApp.tsx

No asumption can be made on how the render() function will be used internally by storybook.

In the case of storybook, they are just discussing a way to make ESLint happy without addressing the root cause of the issue.
They should have called the render function Render pass it props and not multiple arguments and use it as a React component.
It's a design mistake on their end that cannot easily be solved.
On our end we should respect the rules of react and declare a proper component when we need one like:

image

@garronej garronej merged commit 95b1da5 into main Sep 10, 2024
6 checks passed
@garronej garronej deleted the feat-storybook branch September 10, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants