-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
ColorPalette: Make selected color more obvious #20258
ColorPalette: Make selected color more obvious #20258
Conversation
value === color | ||
? { | ||
fill: tinycolor | ||
.mostReadable( color, [ '#111', '#fff' ] ) |
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.
Should we use #000
instead? cc @jasmussen
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.
We should. How much more black could it be? And the answer is none. None more black.
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.
👍 Updated.
Looks great to me. I will remove the feedback label as this works well. Thanks for this @johnwatkins0. It will need a code review but once gets that let's merge! |
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.
selectedIconProps prop seems like a too specific prop (design prop) that is not very semantic for the component's API. That said, since this is an internal component, it's not that important especially cause I'm having trouble finding a better alternative.
Agree on the prop, also seems like it shouldn't be computed so high up the chain. |
Description
This would resolve #17332. Currently when selecting a color from the ColorPalette it can be hard to tell which color you have selected:
This is especially true working with the Text Color format, which doesn't have the "Background Color" indicator that appears other places.
This update adds a new
selectedIconProps
prop to the CircularOptionPicker. This allows extra props to be passed to the check icon that shows over the selected color. We then use this prop to pass down style overrides from the ColorPalette component. The style override uses the tinycolormostReadable
function to determine whether the icon should be white or dark gray.How has this been tested?
Ran Jest tests, which passed when snapshots were updated. Tested locally in the browser using several components that use the ColorPalette, including the table block and pull quote block.
Screenshots
After:
Types of changes
This is a backward-compatible improvement to the display of the color picker.
Checklist: