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

CSF: Remove makeDisplayName option #7900

Closed
wants to merge 3 commits into from

Conversation

shilman
Copy link
Member

@shilman shilman commented Aug 28, 2019

Issue: #7599

What I did

This partially reverts the change made in #7878, keeping the displayName concept, but removing its configurability.

Now we transform the CSF named export using lodash.startCase as the default. If you want other behavior you can use the namedExport.story = { name: 'anything goes' } construct.

    it('should format CSF exports with sensible defaults', () => {
      const testCases = {
        name: 'Name',
        someName: 'Some Name',
        someNAME: 'Some NAME',
        some_custom_NAME: 'Some Custom NAME',
        someName1234: 'Some Name 1234',
        someName1_2_3_4: 'Some Name 1 2 3 4',
      };
      Object.entries(testCases).forEach(([key, val]) => expect(makeDisplayName(key)).toBe(val));
    });

Why I did it

We made hierarchySeparator and hierarchyRootSeparator configurable, and it has been a maintenance nightmare, adding complexity across our codebase and bringing that nightmare to other tools in the Storybook ecosystem.

makeDisplayName is a similar foundational configuration. However since it can contain arbitrary JS code, we expect it will cause an order of magnitude MORE headaches. At the very least we're not ready to sign up for it without a lot more evidence that it's necessary.

How to test

yarn jest --testPathPattern=client_api.test.ts

@shilman shilman added maintenance User-facing maintenance tasks csf BREAKING PRERELEASE labels Aug 28, 2019
@shilman shilman added this to the 5.2.0 milestone Aug 28, 2019
@vercel
Copy link

vercel bot commented Aug 28, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

@ndelangen
Copy link
Member

Yeah we should wait adding this configuration until enough requests have come in with actual use cases.

This seems like a smart move to me.

@shilman shilman requested a review from tmeasday August 28, 2019 14:59
@shilman
Copy link
Member Author

shilman commented Aug 29, 2019

Closing this in favor of #7901 which improves upon this change

@shilman shilman closed this Aug 29, 2019
@stof stof deleted the 7599-remove-displayname-option branch October 4, 2019 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
csf maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants