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

Add easy-to-use options to storySort #9188

Merged
merged 3 commits into from
Dec 21, 2019

Conversation

JohnAlbin
Copy link
Contributor

Issue: #9185

What I did

Adds the storySort object parameter described in #9185.

How to test

  • Tests are included
  • An existing example is updated
  • Documentation is updated

@vercel
Copy link

vercel bot commented Dec 18, 2019

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/17ss924k9
🌍 Preview: https://monorepo-git-fork-johnalbin-storysort.storybook.now.sh

@vercel vercel bot temporarily deployed to staging December 18, 2019 18:35 Inactive
MIGRATION.md Outdated
@@ -256,7 +256,7 @@ For example, here's how to sort by story ID using `storySort`:
addParameters({
options: {
storySort: (a, b) =>
a[1].kind === b[1].kind ? 0 : a[1].id.localeCompare(b[1].id, { numeric: true }),
a[1].kind === b[1].kind ? 0 : a[1].id.localeCompare(b[1].id, undefined, { numeric: true }),
Copy link
Contributor Author

@JohnAlbin JohnAlbin Dec 18, 2019

Choose a reason for hiding this comment

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

FYI, in localeCompare(), the options array is the 3rd parameter, not the 2nd one. MDN shows an example usage of "2".localeCompare("10", undefined, {numeric: true});

Copy link
Member

Choose a reason for hiding this comment

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

😱

Copy link
Member

Choose a reason for hiding this comment

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

🙀

@shilman
Copy link
Member

shilman commented Dec 18, 2019

This looks pretty awesome @JohnAlbin! 🙌

We're out of time on 5.3, but it's now in the queue for 6.0 which will start in early Jan. Thanks for the contribution ... and being patient!

@JohnAlbin
Copy link
Contributor Author

We're out of time on 5.3, but it's now in the queue for 6.0 which will start in early Jan.

Yep. I saw the announcement in Discord this week. No worries.

@JohnAlbin JohnAlbin changed the title Story sort Add easy-to-use options to storySort Dec 19, 2019
@ndelangen ndelangen changed the base branch from next to next-6.0.0 December 21, 2019 19:38
@ndelangen ndelangen self-assigned this Dec 21, 2019
options: {
storySort: {
method: 'alphabetical', // Optional, defaults to 'configure'.
sort: ['Intro', 'Components'], // Optional, defaults to [].

Choose a reason for hiding this comment

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

I see the PR merged in next-6.0.0, but not seeing the changes reflected there. I wanted to point out the sort key here is wrong. The storySort method in this PR uses order to override the order.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this doc seems to be outdated. There's order in tests.

Copy link
Member

Choose a reason for hiding this comment

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

@hasparus would you be able to make a PR fixing the docs for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

yup, no biggie

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants