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: Autotitle fix multiple dots and handle stories.js #21840

Merged
merged 11 commits into from
Nov 28, 2023

Conversation

agriffis
Copy link
Contributor

@agriffis agriffis commented Mar 30, 2023

Note: This is easier to review the two commits in sequence rather than all changes simultaneously. In particular the commit messages may be helpful.

Also, it's possible you'll prefer separate pull requests for the fix and feature, but I'm lazy and taking my chances.

What I did

Fix an auto-titling bug when the filename contains multiple dots, for example button.group.stories.js would dumb down to button

Handle stories.js as a meaningless filename for auto-titling, similarly to index.stories.js

How to test

Added new tests for both the fix and the feature.

Why this matters to me

I prefer stories.tsx rather than index.stories.tsx to avoid tab-completion ambiguities. But I'd also like to use auto-titling.

I found the bug while auditing the code prior to adding the feature.

Ideally, I think we'd be able to supply an autoTitle function in the storybook config for customization, but that's a higher bar because of figuring out the best API, updating docs, etc.

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • If this PR should be tested against many or all sandboxes,
    make sure to add the ci:merged or ci:daily GH label to it.
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

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

Can you add some tests with windows paths?

@agriffis agriffis force-pushed the fix/auto-title-dotted-component branch from 846954a to f50dca1 Compare July 28, 2023 16:21
@agriffis
Copy link
Contributor Author

@ndelangen I'm just getting back to this after a long time.

I'm not sure this really needs a separate test on Windows, but I added one for completeness. Let me know what you think. Thanks!

@ndelangen
Copy link
Member

@valentinpalkovic could you have a look at this? 🙏

@valentinpalkovic valentinpalkovic self-assigned this Jul 31, 2023
@agriffis agriffis force-pushed the fix/auto-title-dotted-component branch from f50dca1 to 34f8281 Compare August 15, 2023 15:47
@agriffis
Copy link
Contributor Author

@valentinpalkovic This had fallen out of date, so I rebased it to the latest next. Do you think you'll have a chance to look at it soon?

@agriffis agriffis force-pushed the fix/auto-title-dotted-component branch from 34f8281 to 7e3178d Compare August 19, 2023 23:16
@agriffis
Copy link
Contributor Author

Rebased again and tweaked considering #23852

@agriffis
Copy link
Contributor Author

agriffis commented Sep 1, 2023

@ndelangen @valentinpalkovic Did this get lost/forgotten?

@ndelangen ndelangen changed the title Fix and feature for auto-titling DocsAddon: Auto-title bug fix and enhancement Sep 5, 2023
@yannbf
Copy link
Member

yannbf commented Sep 19, 2023

Hey @agriffis sorry about that! We are currently very busy (and some of us are on vacation) so it might take us time to go through the PRs.
Would you mind updating the branch with the latest from next and fixing CI? Some steps like unit tests and linting are failing. Once you're done, please ping Norbert. Thank you!

@ndelangen
Copy link
Member

ndelangen commented Sep 19, 2023

I updated the branch for you. Please reach out if you need assistance resolving the unit tests / linting problems.
Thank you @agriffis 🙏

The stripExtension() function aggressively removed everything after the
first dot in the filename. It should remove only (for example) .js or
.stories.js or .story.js

See the new test in autoTitle.test.ts for example.

Additionally the stripExtension() function was pulling double-duty by
cleaning up an empty leading string, a leftover from calling
pathJoin([titlePrefix, suffix]) with empty titlePrefix. Handle this
properly in the earlier code instead of hacking stripExtension().
@agriffis agriffis force-pushed the fix/auto-title-dotted-component branch from 6afe0e3 to 59fc329 Compare September 20, 2023 20:15
@agriffis
Copy link
Contributor Author

@ndelangen @yannbf I rebased to next and added an additional commit for the fix, along with an additional unit test.

@ndelangen
Copy link
Member

I think this looks good, but will defer to either @shilman or @JReinhold to approve and merge this.

@valentinpalkovic
Copy link
Contributor

Reminder @JReinhold, @shilman

@agriffis
Copy link
Contributor Author

I hope it's okay to ping here. This PR seems relatively small, and I'd love to see it merged rather than languish.

@shilman shilman changed the title DocsAddon: Auto-title bug fix and enhancement CSF: Auto-title bug fix and enhancement Nov 1, 2023
@shilman shilman added the csf3 label Nov 1, 2023
@shilman shilman changed the base branch from next to release-8-0 November 1, 2023 08:59
@shilman
Copy link
Member

shilman commented Nov 1, 2023

@agriffis thanks for making this & for your patience on getting it merged! the change looks good to me, but it's also a technically breaking change -- you can see this from our chromatic changes where we see filenames like attribute-selectors.component.stories.ts result in new stories. i'm going to give the core team a chance to object & otherwise we'll merge this as a breaking change for 8.0. thank you!!! 🙏

@shilman shilman changed the title CSF: Auto-title bug fix and enhancement CSF: Autotitle bug fix and enhancement Nov 1, 2023
@yannbf yannbf added ci:daily Run the CI jobs that normally run in the daily job. and removed ci:normal labels Nov 2, 2023
@yannbf
Copy link
Member

yannbf commented Nov 2, 2023

@shilman can you review the Chromatic changes? This definitely affected the angular sandboxes, and for the Storybook UI there were big naming changes that I don't know if it's related to this PR.

@shilman
Copy link
Member

shilman commented Nov 2, 2023

Good catch @yannbf -- I looked at the Angular changes which look good. But the UI storybook changes looks like a regression.

@ndelangen ndelangen removed their assignment Nov 27, 2023
@ndelangen ndelangen changed the base branch from release-8-0 to next November 28, 2023 15:07
@ndelangen
Copy link
Member

@shilman Can we proceed with this PR?

@shilman shilman changed the title CSF: Autotitle bug fix and enhancement CSF: Autotitle fix multiple dots and handle stories.js Nov 28, 2023
@shilman shilman merged commit ca659bd into storybookjs:next Nov 28, 2023
11 of 13 checks passed
@github-actions github-actions bot mentioned this pull request Nov 28, 2023
36 tasks
@github-actions github-actions bot mentioned this pull request Dec 7, 2023
44 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BREAKING CHANGE ci:daily Run the CI jobs that normally run in the daily job. core csf3 feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants