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

Switch the custom color control to a text link, fix padding for color picker. #13708

Merged
merged 8 commits into from
Feb 19, 2019

Conversation

kjellr
Copy link
Contributor

@kjellr kjellr commented Feb 6, 2019

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

original-02


original-01


After

new-02


new-01


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 kjellr added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. labels Feb 6, 2019
@kjellr kjellr self-assigned this Feb 6, 2019
@kjellr kjellr requested review from mapk and jasmussen February 6, 2019 20:48
Copy link
Contributor

@mapk mapk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🚢

@gziolo
Copy link
Member

gziolo commented Feb 7, 2019

There are 4 failing unit tests which fail because HTML markup has changed. Related docs on how to update snapshots:
https://github.com/WordPress/gutenberg/blob/master/docs/contributors/testing-overview.md#snapshot-testing

@youknowriad youknowriad added the Needs Accessibility Feedback Need input from accessibility label Feb 7, 2019
@gziolo gziolo added this to the 5.1 (Gutenberg) milestone Feb 7, 2019
@kjellr
Copy link
Contributor Author

kjellr commented Feb 7, 2019

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.


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. 🚢

Yep. For now, this actually follows the behavior for the "Visibility" and "Publish" state popovers:

screen shot 2019-02-07 at 10 55 54 am


I'll hold off for some a11y 👀 before merging. Thanks, all!

@mapk
Copy link
Contributor

mapk commented Feb 11, 2019

@LukePettway Can we get some a11y eyes on this before merging?

@Luciaisacomputer
Copy link
Contributor

@mapk I can take a look today 🌝

@Luciaisacomputer
Copy link
Contributor

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.

@kjellr
Copy link
Contributor Author

kjellr commented Feb 18, 2019

Hey @LukePettway — just wanted to see if you'd had a moment to review this with NVDA? I really appreciate the help. 🙂

@afercia
Copy link
Contributor

afercia commented Feb 18, 2019

I'd suggest to remove the tooltip, as it's now pointless :)

screenshot 2019-02-18 at 22 33 44

@mapk @LukePettway this change shouldn't impact a11y in any way as it touches only the button and CSS.

@Luciaisacomputer
Copy link
Contributor

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.

@kjellr
Copy link
Contributor Author

kjellr commented Feb 19, 2019

Thanks, @afercia and @LukePettway. I've removed the tooltip, and I'll get this in as soon as those tests pass. 👍

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look good as commented earlier 👍

@kjellr
Copy link
Contributor Author

kjellr commented Feb 19, 2019

Thanks, everyone!

@kjellr kjellr merged commit 8e18caf into master Feb 19, 2019
@kjellr kjellr deleted the update/custom-color-control-as-text-link branch February 19, 2019 15:30
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
… 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.
youknowriad pushed a commit that referenced this pull request Mar 6, 2019
… 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants