CSF: Autotitle fix multiple dots and handle stories.js #21840
Conversation
agriffis
force-pushed
the
fix/auto-title-dotted-component
branch
from
846954a
to
f50dca1
Compare
|
@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! |
|
@valentinpalkovic could you have a look at this? 🙏 |
agriffis
force-pushed
the
fix/auto-title-dotted-component
branch
from
f50dca1
to
34f8281
Compare
|
@valentinpalkovic This had fallen out of date, so I rebased it to the latest |
agriffis
force-pushed
the
fix/auto-title-dotted-component
branch
from
34f8281
to
7e3178d
Compare
|
Rebased again and tweaked considering #23852 |
|
@ndelangen @valentinpalkovic Did this get lost/forgotten? |
ndelangen
changed the title
|
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. |
|
I updated the branch for you. Please reach out if you need assistance resolving the unit tests / linting problems. |
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
force-pushed
the
fix/auto-title-dotted-component
branch
from
6afe0e3
to
59fc329
Compare
|
@ndelangen @yannbf I rebased to next and added an additional commit for the fix, along with an additional unit test. |
|
I think this looks good, but will defer to either @shilman or @JReinhold to approve and merge this. |
|
Reminder @JReinhold, @shilman |
|
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
changed the title
|
@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 |
shilman
changed the title
yannbf
added
ci:daily
|
@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. |
|
Good catch @yannbf -- I looked at the Angular changes which look good. But the UI storybook changes looks like a regression. |
|
@shilman Can we proceed with this PR? |
shilman
changed the title
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.jswould dumb down tobuttonHandle
stories.jsas a meaningless filename for auto-titling, similarly toindex.stories.jsHow to test
Added new tests for both the fix and the feature.
Why this matters to me
I prefer
stories.tsxrather thanindex.stories.tsxto 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
autoTitlefunction in the storybook config for customization, but that's a higher bar because of figuring out the best API, updating docs, etc.Checklist
MIGRATION.MD
Maintainers
make sure to add the
ci:mergedorci:dailyGH label to it.["cleanup", "BREAKING CHANGE", "feature request", "bug", "documentation", "maintenance", "dependencies", "other"]