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

Addon-docs Props: Convert default prop value to string #9525

Merged
merged 2 commits into from
Jan 18, 2020

Conversation

atanasster
Copy link
Member

Issue: #9394

What I did

  • check if the "summary" prop (used for default value) has a toString method and use it to convert to string.
  • added a boolean prop with default value to Button.tsx in cra-ts-kitchen-sink

How to test

use cra-ts-kitchen-sink

@vercel
Copy link

vercel bot commented Jan 18, 2020

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/olnjco00g
✅ Preview: https://monorepo-git-fork-atanasster-proptable-summary-value-tostring.storybook.now.sh

@atanasster atanasster changed the title proptable: convert default prop to string proptable: convert default prop value to string Jan 18, 2020
@shilman shilman added block: props bug patch:yes Bugfix & documentation PR that need to be picked to main branch labels Jan 18, 2020
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shilman shilman changed the title proptable: convert default prop value to string Addon-docs Props: Convert default prop value to string Jan 18, 2020
@shilman shilman merged commit dfffc21 into storybookjs:next Jan 18, 2020
@shilman shilman deleted the proptable-summary-value-tostring branch January 18, 2020 10:21
@shilman
Copy link
Member

shilman commented Jan 18, 2020

@patricklafrance @tmeasday Can you think of any places where this would cause problems?

@patricklafrance
Copy link
Member

patricklafrance commented Jan 18, 2020

I don't think so.

That being said, why do this in the components instead of the code that transform react-docgen data code to display data?

Feels hacky.

Should also be done in a dedicated TypeScript transformation pipeline since it only happens for TS.

@patricklafrance
Copy link
Member

BTW I believe settings react-docgen-typescript savePropValueAsString prop to true would be a better fix:

https://github.com/styleguidist/react-docgen-typescript#parseroptions

@atanasster
Copy link
Member Author

@patricklafrance thanks for the feedback

Here are a few thoughts on my side

  • react-docgen-typescript can be installed manually, or a different tool could be used (rumors are react-docgen 5 would support typescript)
  • the PropValue component is the place where the value is needed as string, seems to me correct place to ensure it is actually converted to a string
  • i think we should leave proptables as is, and allow them to be used by other code - ie the knobs-2 addon could use the prop tables

@patricklafrance
Copy link
Member

patricklafrance commented Jan 18, 2020

Hey @atanasster

You’re right the PropValue is a correct place to ensure it’s a string. What I don’t like about it is that currently all the code that take care of these concerns is in the data transformation layer, not in the rendering layer. This is splitting the same concern in 2 places which makes the code harders to maintain if we continue this way.

A future refactor in 6.0 might change this, but in the current state, this is how it’s been built.

@atanasster
Copy link
Member Author

Thanks @patricklafrance - you are right, lets work on refactoring that part for 6?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
block: props bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants