-
Notifications
You must be signed in to change notification settings - Fork 47
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
Makes name argument for 'toId' optional #17
Makes name argument for 'toId' optional #17
Conversation
src/index.ts
Outdated
`${sanitizeSafe(kind, 'kind')}--${sanitizeSafe(name, 'name')}`; | ||
export const toId = (kind: string, name?: string) => | ||
`${sanitizeSafe(kind, 'kind')}${ | ||
typeof name === 'string' ? `--${sanitizeSafe(name, 'name')}` : '' |
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 think it's enough to check if name
exists here. You can skip typecheking here. WDYT?
name ? `--${sanitizeSafe(name, 'name')}` : ''
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, I thought so too, but there's a test case for throwing an exception should the argument (name) be empty string. Without typecheking, an empty string would never be passed to sanitizeSafe
and the function will not throw. Wasn't sure, if I should keep this behavior (throwing an exception)... Then again, the whole issue here is making the name arg optional...
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 went ahead and allowed ''
as story. We're intentionally adding support for this, so the test had to change.
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.
LGTM, makes sense I think
Will this be merged soon? We need this update to fix the related Storybook bug in our implementation. |
🚀 PR was released in |
This PR allows generating storybook ID only from a component/kind and would help getting this fixed.