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

docs: migrate stories to use LiveEdit, add TS inner types to those stories #2080

Merged

Conversation

YossiSaadi
Copy link
Contributor

Base automatically changed from feat/yossi/LiveEdit---Fit-render-parse-from-CSF-in-LiveEdit-to-parse-all-cases-correctly-6494355245 to master April 21, 2024 22:57
@YossiSaadi
Copy link
Contributor Author

Exceptions:

  • Checkbox: disabled for SingleCheckbox story
  • All deprecated
  • All components beneath Toggle

@YossiSaadi YossiSaadi changed the title docs: migrate stories to use LiveEdit, add TS to all stories docs: migrate stories to use LiveEdit, add TS inner types to those stories Apr 22, 2024
@YossiSaadi YossiSaadi marked this pull request as ready for review April 22, 2024 00:10
@YossiSaadi YossiSaadi requested a review from a team as a code owner April 22, 2024 00:10
@@ -55,6 +62,11 @@ export const SingleCheckbox = {
}
/>
),

name: "Single checkbox"
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you remove the name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Storybook automatically parses the name from the exported story and uses its naming convention.
So 99% of our stories "name" attribute is the same as Storybook would parse.
The cases where it's different it is usually something unnoticeable that wasn't inserted by purpose, such as a lowercased story name

@YossiSaadi
Copy link
Contributor Author

PR would fail until #2090 is merged because of decorator types issue from createMetaSettings function.
StoryObj<typeof SomeComponent> does not fit the current returned type for decorators

@@ -299,9 +302,7 @@ export const AsyncOptions = {
<Dropdown asyncOptions={fetchUserOptions} placeholder="Async options" cacheOptions defaultOptions />
</div>
);
},

name: "Async Dropdown"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you also need to add it this one back :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed it intentionally and changed the name of the exported story to AsyncDropdown instead of AsyncOptions, so the name of the story now should be the same as the old one

@@ -92,11 +105,16 @@ export const DifferentIcon = {
</Menu>
</MenuButton>
),

name: "Different Icon"
Copy link
Contributor

Choose a reason for hiding this comment

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

Also need to be reverted :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Storybook uses lodash's startCase method:
https://lodash.com/docs/#startCase
so export const DifferentIcon would result in a story with the name "Different Icon", so no need for the name here as it would result the same

@@ -112,11 +130,16 @@ export const WithText = {
</MenuButton>
</div>
),

name: "With Text"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as the above about startCase

@@ -170,6 +204,11 @@ export const CustomTriggerElement = {
</MenuButton>
);
},

name: "Custom Trigger Element"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this one also is already using a startCase convention

@YossiSaadi
Copy link
Contributor Author

@shaharzil the only changed story here is the AsyncDropdown that for some reason Chromatic (due to the name change) consider as a new story, though I had an AsyncOptions story with name: Async Dropdown.

The rest had no change in the build: https://monday.chromatic.com/build?appId=62037c72e0e4d6004a9a450f&number=5331, meaning all the names remained the same (which means what we've discussed - that the parsing of the names remains the same, as if we didn't changed anything in this logic)

@YossiSaadi YossiSaadi requested a review from shaharzil May 1, 2024 11:27
@YossiSaadi YossiSaadi merged commit f92fbc1 into master May 2, 2024
10 checks passed
@YossiSaadi YossiSaadi deleted the docs/yossi/Align-all-TS-stories--allow-LiveEdit-6494680289 branch May 2, 2024 14:21
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