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

Core: Ensure node name does not contain leading/trailing whitespace #13275

Merged
merged 3 commits into from
Dec 1, 2020

Conversation

ghengeveld
Copy link
Member

@ghengeveld ghengeveld commented Nov 25, 2020

Issue: #13251

What I did

Primarily, this trims each part of the title (aka kind), after splitting it on /:

const groups = kind.split('/').map((part) => part.trim());

The .map(part => part.trim()) is what's added. Everything else in this PR is refactoring. I'll leave comments to clarify.

Pro tip: ignore whitespace changes when reviewing this 👍

How to test

  • Is this testable with Jest or Chromatic screenshots? Yes, a unit test should be added.
  • Does this need a new example in the kitchen sink apps? No
  • Does this need an update to the documentation? No

If your answer is yes to any of these, please make sure to include it in your PR.

@ghengeveld ghengeveld changed the title Core: Trim whitespace in story path parts, to ensure component names will not contain leading/trailing whitespace Core: Ensure component name does not contain leading/trailing whitespace Nov 25, 2020
@ghengeveld ghengeveld changed the title Core: Ensure component name does not contain leading/trailing whitespace Core: Ensure node name does not contain leading/trailing whitespace Nov 25, 2020
@tmeasday
Copy link
Member

I'm surprised this didn't involve any test changes. Do we need a test for this case?

@ghengeveld
Copy link
Member Author

I'm surprised this didn't involve any test changes. Do we need a test for this case?

Yes we do, but I'm not sure where to add it. This whole lib is untested.

@tmeasday
Copy link
Member

@ghengeveld
Copy link
Member Author

@tmeasday I added a test.

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

LGTM

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

Successfully merging this pull request may close these issues.

4 participants