-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
fix: techOrder names can be camelCased. #4277
Conversation
src/js/utils/to-title-case.js
Outdated
@@ -21,3 +21,7 @@ function toTitleCase(string) { | |||
} | |||
|
|||
export default toTitleCase; | |||
|
|||
export function titleCaseEquals(str1, str2) { |
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.
JSDoc?
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.
Yes. Also, we need some tests, probably.
should we update any other if checks to use the new titleCaseEquals function? |
@brandonocasey I think updates to current code should be done in another PR. |
@gkatsev OK |
* Compares the TitleCase versions of the two strings for equality. | ||
* | ||
* @param {string} str1 | ||
* The first string to compare |
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 we want a newline between this param and the other one for consistency
assert.ok(titleCaseEquals('foo', 'foo'), 'foo equals foo'); | ||
assert.ok(titleCaseEquals('foo', 'Foo'), 'foo equals Foo'); | ||
assert.ok(titleCaseEquals('Foo', 'foo'), 'Foo equals foo'); | ||
assert.ok(titleCaseEquals('Foo', 'Foo'), 'Foo equals Foo'); |
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.
maybe some negative tests with notOK
too? fOO
& foo
etc
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
In the new middleware work, the way that new sources were loaded was refactored. We also recently made techs and components work either TitleCased or camelcased. There was one comparison that didn't do the proper check and cause the tech to be reloaded, even if the two techs were the same.