Switch the custom color control to a text link, fix padding for color picker.#13708
Switch the custom color control to a text link, fix padding for color picker.#13708
Conversation
Changes the custom color control into a text link instead of a multi-colored palette icon. Updates styles to properly line it up to the left of the clear button.
kjellr
added
[Type] Enhancement
kjellr
requested review from
aduth,
ajitbohra,
chrisvanpatten,
gziolo,
jorgefilipecosta,
noisysocks,
oandregal,
talldan and
youknowriad
as code owners
mapk
left a comment
mapk
left a comment
There was a problem hiding this comment.
Looks and works great! Tested on smaller and larger screens without any issues. I do feel a little awkward with a link opening up a popover, which feels more like a button interaction. But we can always iterate if needed. 🚢
|
There are 4 failing unit tests which fail because HTML markup has changed. Related docs on how to update snapshots: |
Thanks, @gziolo! Should be all set now.
Yep. For now, this actually follows the behavior for the "Visibility" and "Publish" state popovers:
I'll hold off for some a11y 👀 before merging. Thanks, all! |
|
@LukePettway Can we get some a11y eyes on this before merging? |
|
@mapk I can take a look today 🌝 |
|
Initial keyboard testing seems fine so far. I'm having issues with my VM so I can't test NVDA until I get home later today. |
|
Hey @LukePettway — just wanted to see if you'd had a moment to review this with NVDA? I really appreciate the help. 🙂 |
|
I'd suggest to remove the tooltip, as it's now pointless :)
@mapk @LukePettway this change shouldn't impact a11y in any way as it touches only the button and CSS. |
|
Thanks @afercia I did retest this again in Voice Over (since I can't seem to get Gutenberg running on Windows) and it worked. I think I wasn't using Safari 🤣. Everything worked fine. I agree that it shouldn't mess with anything AT related. Everything looks good to me. |
|
Thanks, @afercia and @LukePettway. I've removed the tooltip, and I'll get this in as soon as those tests pass. 👍 |
gziolo
left a comment
gziolo
left a comment
There was a problem hiding this comment.
Code changes look good as commented earlier 👍
|
Thanks, everyone! |
… picker. (#13708) * Change custom color control into a text link. Changes the custom color control into a text link instead of a multi-colored palette icon. Updates styles to properly line it up to the left of the clear button. * Add missing top/bottom padding to color picker controls. * Update snapshot * Remove unnecessary tooltip on the color picker button. * Update snapshot. * Update test + snapshot * Remove unnecessary type declaration for the toggle button.
… picker. (#13708) * Change custom color control into a text link. Changes the custom color control into a text link instead of a multi-colored palette icon. Updates styles to properly line it up to the left of the clear button. * Add missing top/bottom padding to color picker controls. * Update snapshot * Remove unnecessary tooltip on the color picker button. * Update snapshot. * Update test + snapshot * Remove unnecessary type declaration for the toggle button.
7 participants
Fixes #5258.
This PR changes the custom color control from a multi-color palette icon to a text link, lined up next to the "Clear" button. It also adds proper top + bottom padding to the color picker's controls area (This appears to have been a regression at some point).
Before
After