-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
CSF3: Handle auto-title redundant filename #17421
Conversation
☁️ Nx Cloud ReportCI ran the following commands for commit 39b3e6e. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome stuff! A couple comments
) | ||
).toMatchInlineSnapshot(`To/Button`); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the outcome of a test with this?
auto(
'./path/to/button/index.stories.js',
normalizeStoriesEntry({ directory: './path' }, options)
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To/Button/Index
-- do you think we should remove Index
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say so, normally index
is ommited when importing e.g.
import { Button } from './components/Button'
so personally I'd expect a similar behavior. @tmeasday @ndelangen WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, "Index"
is meaningless to everyone, we should ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Thanks gang! 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we also cope with just stories.js
? I feel like that filename would make sense for a folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was an issue in the eslint-plugin-storybook repo about supporting stories.js
files. We never suggest that though, and I believe our internal defaults are all about *.stories.*
. Do you think we should support stories.*
? I've never seen people having something like test.js
in their folders, just index.test.js
or Component.test.js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen it before, but I'm OK with saying it's not something we recommend.
Issue: #15534
What I did
Technically breaking change in experimental auto-title:
Input:
atoms/button/button.stories.js
Atoms/Button/Button
Atoms/Button
Input:
atoms/button/index.stories.js
Atoms/Button/Index
Atoms/Button
See attached migration notes for full explanation
How to test
See attached test