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

Invalid kind 'Кнопки', must include alphanumeric characters #5876

Closed
MerrickGit opened this issue Mar 5, 2019 · 12 comments
Closed

Invalid kind 'Кнопки', must include alphanumeric characters #5876

MerrickGit opened this issue Mar 5, 2019 · 12 comments

Comments

@MerrickGit
Copy link

MerrickGit commented Mar 5, 2019

Describe the bug
When i use cyrillic symbols in the storiesOf, getting the bug.

Code snippets
If applicable, add code samples to help explain your problem.

storiesOf('Кнопки', module)
  .addDecorator(withKnobs)
  .add(
    'Button',
    withInfo(info)(() => (
      <React.Fragment>
        <Button type="primary" label="primary" />
      </React.Fragment>
    ))
  );

Additional context

exports.sanitize = function (string) {
    return string
        .toLowerCase()
        .replace(/[^a-z0-9-]/g, '-')
        .replace(/-+/g, '-')
        .replace(/^-+/, '')
        .replace(/-+$/, '');
};

Error in .replace(/[^a-z0-9-]/g, '-'). We can use only latin titles.

@shilman shilman added this to the 5.0.x milestone Mar 5, 2019
@shilman
Copy link
Member

shilman commented Mar 5, 2019

This is a good one! Any interest in contributing a PR with the fix?

@tmeasday
Copy link
Member

tmeasday commented Mar 5, 2019

This is a tricky one! I'm quite sure what the best fix is. Should we:

A) allow non-latin characters in storyIds? (part of the idea was to restrict them to make them simpler to work with, certainly want to restrict them in some ways)
B) transliterate non-latin chars to latin equivalents?
C) something else?

What did you have in mind @MerrickGit?

@MerrickGit
Copy link
Author

This is a tricky one! I'm quite sure what the best fix is. Should we:

A) allow non-latin characters in storyIds? (part of the idea was to restrict them to make them simpler to work with, certainly want to restrict them in some ways)
B) transliterate non-latin chars to latin equivalents?
C) something else?

What did you have in mind @MerrickGit?

We are creating a banking app and our customers interested in сyrillic stories. I think others company which use storybook also need non-latin symbols, for example, chinese companies.

My vision, Storybook need to allow non-latin characters otherwise these companies will not be able to update to new version.

What do you think about it? @tmeasday @shilman

@Armanio
Copy link
Member

Armanio commented Mar 6, 2019

Fully support comrade @MerrickGit.
It's weird. I get empty string instead of 'сторис'.
If SB not support non-latin symbols must throw error.

@tmeasday
Copy link
Member

tmeasday commented Mar 6, 2019

Of course! We need to support non-latin story names of course! This was just an oversight in the design 🤦‍♂️

I just was wondering because your comment implied you had a solution in mind. But we'll figure something out very soon.

@shilman
Copy link
Member

shilman commented Mar 7, 2019

Discussed with @tmeasday:

  • Change from whitelist chars to blacklist? Expand whitelist? Technically a breaking change.
  • Allow specifying an ID separately from the story name? Probably inconvenient.

==> Find list of chars to expand to

@shilman shilman assigned shilman and unassigned tmeasday and ndelangen Mar 7, 2019
@ndelangen
Copy link
Member

ndelangen commented Mar 7, 2019

Maybe this:

.replace(/['`~!@#$%^&*()_|+-=?;:'",.<>\{\}\[\]\\\/]/gi, "")

@Armanio
Copy link
Member

Armanio commented Mar 7, 2019

@ndelangen like a good solution.

@ndelangen
Copy link
Member

@Armanio I'm not sure if you mean my suggestion is good or bad 😿

@shilman suggested using slugify, which seems top make a lot of sense to me too.

@Armanio
Copy link
Member

Armanio commented Mar 7, 2019

@ndelangen sorry for implicit message. It's not sarcasm, good solution.
Blacklist is better than whitelist.
Of course it's only my opinion.

@shilman
Copy link
Member

shilman commented Mar 8, 2019

ZOMG!! I just released https://github.com/storybooks/storybook/releases/tag/v5.1.0-alpha.2 containing PR #5964 that references this issue. Upgrade today to try it out!

Because it's a pre-release you can find it on the @next NPM tag.

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Mar 8, 2019
@shilman
Copy link
Member

shilman commented Mar 8, 2019

Yay!! I just released https://github.com/storybooks/storybook/releases/tag/v5.0.1 containing PR #5964 that references this issue. Upgrade today to try it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants