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

fix: Theme.name does not match registered name (#6186) #6226

Merged
merged 2 commits into from
Jun 23, 2022
Merged

fix: Theme.name does not match registered name (#6186) #6226

merged 2 commits into from
Jun 23, 2022

Conversation

derwehr
Copy link
Contributor

@derwehr derwehr commented Jun 18, 2022

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

#6186

Proposed Changes

Add a line in defineTheme to convert its name to lower case.

Behavior Before Change

Theme.name did not match registered name, when it included upper case letters.

Behavior After Change

Now it does 🎉

@derwehr derwehr requested a review from a team as a code owner June 18, 2022 16:59
@derwehr derwehr requested a review from cpcallen June 18, 2022 16:59
@cpcallen
Copy link
Contributor

Thanks for this fix. I added a question to the issue, but that is not why I have not merged this yet.

Rather, we're wondering if you would write a unit test for this change. This would be especially helpful at the moment as we're busy migrating core/ to TypeScript in the ts/migration branch, and we want to make sure this change doesn't get lost when that branch gets merged into develop. A unit test—safely outside the core/ directory, would catch such an inadvertent regression.

@derwehr
Copy link
Contributor Author

derwehr commented Jun 23, 2022

Thanks for the review. I added a test for defineTheme normalizing to lowercase.

@BeksOmega
Copy link
Collaborator

Looks perfect! Thank you again for the contribution @derwehr :D

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

Successfully merging this pull request may close these issues.

3 participants