-
Notifications
You must be signed in to change notification settings - Fork 12k
chore: force component and route names to have dashes #467
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
Conversation
Could you add a test 😄 |
250ed62
to
0e103c8
Compare
@@ -16,6 +27,11 @@ module.exports = { | |||
var parsedPath = dynamicPathParser(this.project, entityName); | |||
|
|||
this.dynamicPath = parsedPath; | |||
|
|||
if (!validateName(parsedPath.name)) { | |||
throw 'Invalid name. Names must be camelCase or kabob-case'; |
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.
Did you mean kebab-case
?
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 should know better than to trust my spelling, ever since 3rd grade when I spelled enough "enuff"
Thanks for catching that @gkalpak!
0e103c8
to
5858a5e
Compare
@@ -16,6 +27,11 @@ module.exports = { | |||
var parsedPath = dynamicPathParser(this.project, entityName); | |||
|
|||
this.dynamicPath = parsedPath; | |||
|
|||
if (!validateName(parsedPath.name)) { | |||
throw 'Invalid name. Names must be camelCase or kebab-case'; |
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.
FWIW, the message might be confusing with single-word names, since toast
is technically valid camelCase and kebab-case.
More generally, validateName()
isn't strict at all, which means that all sorts of non-camelCase-or-kebab-case names would pass. (But maybe that's intentional.)
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.
The purpose of this is to enforce the selector to contain a -
so I agree, I'll tweak it to show a more useful message to the user.
I'm aware that the names can contain any variety of capital/lower/dash characters, but that is indeed intentional, as long as the name transformation creates a selector with at least 1 dash, then it will satisfy the HTML spec for custom elements.
I'm open to suggestions on the actual error message, I currently have this:
Names must contain a dash either include a dash or multiCase name. (i.e. multiCase -> multi-case)
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 that's better. "Invalid name" does scare people off.
Change the message, then LGTM 👍 |
5858a5e
to
cd4dcf0
Compare
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
No description provided.