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

Sort the PropsTable rows alphabetically #8830

Closed
wants to merge 2 commits into from

Conversation

Joseph-Whiunui
Copy link

Issue: #8539

What I did

How to test

  • Is this testable with Jest or Chromatic screenshots? Yes Chromatic screenshots
  • Does this need a new example in the kitchen sink apps? No
  • Does this need an update to the documentation? No

If your answer is yes to any of these, please make sure to include it in your PR.

@vercel
Copy link

vercel bot commented Nov 14, 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/6h74r31vp
🌍 Preview: https://monorepo-git-fork-joseph-whiunui-next.storybook.now.sh

@patricklafrance
Copy link
Member

What's you thought about this solution @shilman ?

@domyen
Copy link
Member

domyen commented Nov 14, 2019

I think the order of the prop table should be determined by the order of the props in the user's code. If users want to alphabetize their props they should do that themselves – Storybook shouldn't do that for them.

That said, I can see how alphabetizing would be useful for documentation consumers. Perhaps we can sort the table when you click on the "Name" table heading.

@shilman
Copy link
Member

shilman commented Nov 14, 2019

@patricklafrance I agree with @domyen. This solution might be an improvement on the current behavior, but alphabetizing feels like a patch-over for some underlying problem in one of the libraries we're using. Can we figure out what's going on under the hood and fix that instead?

(That said, thank you @Joseph-Whiunui for providing an improvement on what's there right now! 🙏🙏🙏 )

@ndelangen
Copy link
Member

Some props are more used then others, those should get the most attention.

The alphabetical order could be an OK opt-in option though.

<PropsTable sort="alphabetical" />

?

@patricklafrance
Copy link
Member

I like the idea of making the sorting configurable.

Instead of having predefined sorts, the PropsTable could accept a sort function?

Still need to find the actual problem thought.

@shilman
Copy link
Member

shilman commented Nov 15, 2019

I'd like to focus on "code definition order" as the default and then possibly add an alpha sort on top of that. Making it a function is easy, but makes it harder to serialize as we start making docs more embeddable in other contexts, so it would be great if we could start with something more narrow and expand if the use cases come up.

@Joseph-Whiunui
Copy link
Author

No worries guys it was great to get a look at the OSS process!

@ndelangen
Copy link
Member

@Joseph-Whiunui if you'd like, I've got a a prototype for something pretty revolutionary that could use some help to get finished..

@ndelangen ndelangen added this to the 5.4.0 milestone Nov 16, 2019
@Joseph-Whiunui
Copy link
Author

@ndelangen Sure, I'm happy to take a look.

@ndelangen
Copy link
Member

Would you be able to attend an online meeting maybe? tomorrow?

I'll show you what I've got and what the concepts & goals are.

Are you on our discord yet? https://discord.gg/sMFvFsG

@ndelangen ndelangen self-assigned this Nov 17, 2019
@Joseph-Whiunui
Copy link
Author

It will depend on the time. I'm in NZ so I am available after work (5am-8am GMT) and before (5pm-6pm GMT)

Yes, username is Joseph-Whiunui#7094

@patricklafrance
Copy link
Member

Closing this since #8857 has been merged

@shilman shilman modified the milestones: 5.4.0, 5.3.0 Dec 12, 2019
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