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

Color Picker: Creating a custom color picker #2406

Merged
merged 9 commits into from Aug 18, 2017

Conversation

Projects
None yet
5 participants
@youknowriad
Contributor

youknowriad commented Aug 14, 2017

We definitely need to find a better way to style the "Random color picker" but this PR tries to improve the color-picker with a clear color option and a random color picker.

@youknowriad youknowriad self-assigned this Aug 14, 2017

@youknowriad youknowriad requested review from mtias and jasmussen Aug 14, 2017

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 14, 2017

Contributor

I think this seems good. Thank you for working on this.

Seeing some style issues:

screen shot 2017-08-14 at 16 31 36

And a fun outline:

screen shot 2017-08-14 at 16 30 44

I like the "no swatch" option, btw ;)

Contributor

jasmussen commented Aug 14, 2017

I think this seems good. Thank you for working on this.

Seeing some style issues:

screen shot 2017-08-14 at 16 31 36

And a fun outline:

screen shot 2017-08-14 at 16 30 44

I like the "no swatch" option, btw ;)

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 14, 2017

Codecov Report

Merging #2406 into master will decrease coverage by 0.55%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2406      +/-   ##
==========================================
- Coverage   26.94%   26.38%   -0.56%     
==========================================
  Files         158      161       +3     
  Lines        4881     5462     +581     
  Branches      814      978     +164     
==========================================
+ Hits         1315     1441     +126     
- Misses       3023     3348     +325     
- Partials      543      673     +130
Impacted Files Coverage Δ
blocks/color-palette/index.js 0% <0%> (ø) ⬆️
blocks/library/button/index.js 26.66% <0%> (ø) ⬆️
blocks/library/cover-text/index.js 35.89% <0%> (-0.47%) ⬇️
blocks/api/paste/strip-attributes.js 76.47% <0%> (-23.53%) ⬇️
editor/header/saved-state/index.js 75% <0%> (-10.72%) ⬇️
blocks/library/paragraph/index.js 39.28% <0%> (-7.78%) ⬇️
blocks/library/heading/index.js 18.91% <0%> (-4.9%) ⬇️
blocks/library/table/index.js 30% <0%> (-3.34%) ⬇️
blocks/library/code/index.js 40% <0%> (-2.86%) ⬇️
blocks/library/preformatted/index.js 37.5% <0%> (-2.5%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be9cb7b...cbe8e75. Read the comment docs.

codecov bot commented Aug 14, 2017

Codecov Report

Merging #2406 into master will decrease coverage by 0.55%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2406      +/-   ##
==========================================
- Coverage   26.94%   26.38%   -0.56%     
==========================================
  Files         158      161       +3     
  Lines        4881     5462     +581     
  Branches      814      978     +164     
==========================================
+ Hits         1315     1441     +126     
- Misses       3023     3348     +325     
- Partials      543      673     +130
Impacted Files Coverage Δ
blocks/color-palette/index.js 0% <0%> (ø) ⬆️
blocks/library/button/index.js 26.66% <0%> (ø) ⬆️
blocks/library/cover-text/index.js 35.89% <0%> (-0.47%) ⬇️
blocks/api/paste/strip-attributes.js 76.47% <0%> (-23.53%) ⬇️
editor/header/saved-state/index.js 75% <0%> (-10.72%) ⬇️
blocks/library/paragraph/index.js 39.28% <0%> (-7.78%) ⬇️
blocks/library/heading/index.js 18.91% <0%> (-4.9%) ⬇️
blocks/library/table/index.js 30% <0%> (-3.34%) ⬇️
blocks/library/code/index.js 40% <0%> (-2.86%) ⬇️
blocks/library/preformatted/index.js 37.5% <0%> (-2.5%) ⬇️
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be9cb7b...cbe8e75. Read the comment docs.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 17, 2017

Contributor

Should we merge this? What's left?

Contributor

youknowriad commented Aug 17, 2017

Should we merge this? What's left?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 17, 2017

Contributor

I like it, I think it's good!

Matías wasn't as much a fan of the red circle with line through it as I was (I'm the one that suggested it). He suggested a "Remove color" link.

So instead of:

screen shot 2017-08-17 at 10 44 01

maybe something like this?

screen-shot-2017-08-17-at-10 44 01

Maybe "Remove" is enough?

But either way I have no strong opinion, I think it's good to get this in soon though.

Contributor

jasmussen commented Aug 17, 2017

I like it, I think it's good!

Matías wasn't as much a fan of the red circle with line through it as I was (I'm the one that suggested it). He suggested a "Remove color" link.

So instead of:

screen shot 2017-08-17 at 10 44 01

maybe something like this?

screen-shot-2017-08-17-at-10 44 01

Maybe "Remove" is enough?

But either way I have no strong opinion, I think it's good to get this in soon though.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 17, 2017

Contributor

@jasmussen any clue why the first set of colors has the rainbow-circle slightly cut off at the top but not the bottom one?

Contributor

mtias commented Aug 17, 2017

@jasmussen any clue why the first set of colors has the rainbow-circle slightly cut off at the top but not the bottom one?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Aug 17, 2017

Contributor

any clue why the first set of colors has the rainbow-circle slightly cut off at the top but not the bottom one?

Huh, that is a doozy. I sense that it must be some subpixel stuff, but I can't figure it out. Check this out, though:

huh

If you apply a 20px top margin on the first container of blogs, it's uncropped. If that margin is 0-19px, it's cropped.

Contributor

jasmussen commented Aug 17, 2017

any clue why the first set of colors has the rainbow-circle slightly cut off at the top but not the bottom one?

Huh, that is a doozy. I sense that it must be some subpixel stuff, but I can't figure it out. Check this out, though:

huh

If you apply a 20px top margin on the first container of blogs, it's uncropped. If that margin is 0-19px, it's cropped.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 17, 2017

Contributor

@afercia added a few aria-roles, but I wonder what you think of aria-current and its usage. Would it work if the group is not a list? Is there a better approach for this one? Should we also add a label to the main palette wrapper stating which color is currently selected?

Contributor

mtias commented Aug 17, 2017

@afercia added a few aria-roles, but I wonder what you think of aria-current and its usage. Would it work if the group is not a list? Is there a better approach for this one? Should we also add a label to the main palette wrapper stating which color is currently selected?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Aug 18, 2017

Contributor

@mtias I added an eslint-disable comment. I think we should merge and reconsider once @afercia gets back from AFK :)

Contributor

youknowriad commented Aug 18, 2017

@mtias I added an eslint-disable comment. I think we should merge and reconsider once @afercia gets back from AFK :)

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 18, 2017

Contributor

👍

Contributor

mtias commented Aug 18, 2017

👍

@mtias

mtias approved these changes Aug 18, 2017

@youknowriad youknowriad merged commit d35adaf into master Aug 18, 2017

2 of 3 checks passed

codecov/project 26.38% (-0.56%) compared to be9cb7b
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the try/custom-color-picker branch Aug 18, 2017

className={ className }
style={ style }
onClick={ () => onChange( value === color ? undefined : color ) }
aria-label={ __( 'Color: ' ) + color }

This comment has been minimized.

@aduth

aduth Aug 23, 2017

Member

We should use variable substitution here:

sprintf( __( 'Color: %s', color )
  • Do not leave leading or trailing whitespace in a translatable phrase.
  • Add the variables as placeholders to the string as in some languages the placeholders change position.

https://developer.wordpress.org/themes/functionality/internationalization/#best-practices-for-writing-strings

@aduth

aduth Aug 23, 2017

Member

We should use variable substitution here:

sprintf( __( 'Color: %s', color )
  • Do not leave leading or trailing whitespace in a translatable phrase.
  • Add the variables as placeholders to the string as in some languages the placeholders change position.

https://developer.wordpress.org/themes/functionality/internationalization/#best-practices-for-writing-strings

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Aug 30, 2017

Contributor

@afercia added a few aria-roles, but I wonder what you think of aria-current and its usage. Would it work if the group is not a list? Is there a better approach for this one? Should we also add a label to the main palette wrapper stating which color is currently selected?

Bit late, but it's on my list 🙂 @mtias I think aria-current is probably not the best option here, but I haven't checked in depth. Will try to test the new color picker soon.

Contributor

afercia commented Aug 30, 2017

@afercia added a few aria-roles, but I wonder what you think of aria-current and its usage. Would it work if the group is not a list? Is there a better approach for this one? Should we also add a label to the main palette wrapper stating which color is currently selected?

Bit late, but it's on my list 🙂 @mtias I think aria-current is probably not the best option here, but I haven't checked in depth. Will try to test the new color picker soon.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Aug 30, 2017

Contributor

Yes, I read contradicting statements around aria-current. :)

Contributor

mtias commented Aug 30, 2017

Yes, I read contradicting statements around aria-current. :)

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 5, 2017

Contributor

Yes, I read contradicting statements around aria-current. :)

Yep, always better read the documentation before relying on other sources... 🙂

https://www.w3.org/TR/wai-aria-1.1/#aria-current

Authors should not use the aria-current attribute as a substitute for aria-selected etc. etc.

Also, aria-current can have only the values specified in the recommendation:
page, step, location, date, time, true, and false
Instead, currently it's aria-current="Selected color" which is invalid...

However, I'd say in this case the best option would probably be aria-pressed
https://www.w3.org/TR/wai-aria-1.1/#aria-pressed

Indicates the current "pressed" state of toggle buttons. Will open a new issue, if there isn't one already.

These controls are actually buttons and only one button at a time can be selected within its set, so they're "toggle buttons".

Contributor

afercia commented Sep 5, 2017

Yes, I read contradicting statements around aria-current. :)

Yep, always better read the documentation before relying on other sources... 🙂

https://www.w3.org/TR/wai-aria-1.1/#aria-current

Authors should not use the aria-current attribute as a substitute for aria-selected etc. etc.

Also, aria-current can have only the values specified in the recommendation:
page, step, location, date, time, true, and false
Instead, currently it's aria-current="Selected color" which is invalid...

However, I'd say in this case the best option would probably be aria-pressed
https://www.w3.org/TR/wai-aria-1.1/#aria-pressed

Indicates the current "pressed" state of toggle buttons. Will open a new issue, if there isn't one already.

These controls are actually buttons and only one button at a time can be selected within its set, so they're "toggle buttons".

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 5, 2017

Contributor

@afercia the most important to me is how—and where—to announce which color is currently selected.

Contributor

mtias commented Sep 5, 2017

@afercia the most important to me is how—and where—to announce which color is currently selected.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 5, 2017

Contributor

@mtias that's obvious 🙂

https://www.w3.org/TR/wai-aria-1.1/#aria-current

Any value not included in the list of allowed values should be treated by assistive technologies as if the value true had been provided.

What screen readers actually announce is "current item", as if the value provided was "true".

screen shot 2017-09-05 at 19 14 47

To recap:

Color {brief pause} number eight ed one f c, current item, button

where

  • the first part comes from the aria-label value Color: #8ed1fc
  • "current item" comes from aria-current as if it was "true", the actual value "Selected color" gets totally ignored
  • "button" is the element type screen readers announce by default
Contributor

afercia commented Sep 5, 2017

@mtias that's obvious 🙂

https://www.w3.org/TR/wai-aria-1.1/#aria-current

Any value not included in the list of allowed values should be treated by assistive technologies as if the value true had been provided.

What screen readers actually announce is "current item", as if the value provided was "true".

screen shot 2017-09-05 at 19 14 47

To recap:

Color {brief pause} number eight ed one f c, current item, button

where

  • the first part comes from the aria-label value Color: #8ed1fc
  • "current item" comes from aria-current as if it was "true", the actual value "Selected color" gets totally ignored
  • "button" is the element type screen readers announce by default
@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 5, 2017

Contributor

So Color: value, current item would make sense then? How would you know, without getting into the individual buttons, which color is selected?

Contributor

mtias commented Sep 5, 2017

So Color: value, current item would make sense then? How would you know, without getting into the individual buttons, which color is selected?

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 5, 2017

Contributor

I think our communication is failing a bit 🙁

Contributor

afercia commented Sep 5, 2017

I think our communication is failing a bit 🙁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment