-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
ColorPicker: Replace hardcoded "blue" with theme color #36153
Conversation
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.
Code-wise, the changes LGTM!
The highlighted text in the color picker follows the selected theme:
color-picker-theme-color.mp4
Before merging, though, I'd like to get a blessing from @jasmussen too (thank you, Joen!)
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.
Code looks good, thank you! Definitely use the theme color for this in similar cases as well. I do suspect some time in the future, we'll want to revisit some of those color schemes for contrast and such, but that's entirely separate.
Note that I wasn't actually able to test this, I got an error:
But I'm almost convinced that error was present in trunk at the time (14 days ago), and will be fixed if the PR is rebased.
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.
Thank you for your review @jasmussen ! I mainly wanted to make sure that it's fine to use the admin theme color in this scenario :)
Note that I wasn't actually able to test this, I got an error:
I don't think this PR could have introduced this error, since it's a very simple JS (CSS) change. FWIW, I carried my tests via this link
Yep, I recall those errors happening to unrelated branches. This looks good. |
Description
ColorPicker
used a hardcodedblue
color for its input prefixes, which can look quite random when users have different admin theme colors set. This PR replaces the hardcoded color with the UI theme color.How has this been tested?
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.js
files for terms that need renaming or removal).