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 improvements, first round #2680

Merged
merged 8 commits into from Sep 29, 2017

Conversation

Projects
None yet
2 participants
@afercia
Contributor

afercia commented Sep 6, 2017

This PR tries to address a first round of improvements to the Color picker buttons.

  • changes aria-current to aria-pressed, see screenshot below to check how this the way buttons and itneraction with them get announced
  • fix invalid HTML: buttons can't contain divs
  • add missing aria-label on the "remove color" button
  • make the Custom color picker toggle a real toggle that opens/closes the popover and uses aria-expanded
  • fix the focus style on the Custom color picker: adds an inner span and applies overflow: hidden on it instead of applying it on the button (I guess this button with gradients needs to be tested a bit in IE 11 / Edge)

Fixes #2679

@afercia afercia requested a review from mtias Sep 6, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 6, 2017

Codecov Report

Merging #2680 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2680      +/-   ##
==========================================
- Coverage   33.77%   33.74%   -0.03%     
==========================================
  Files         191      191              
  Lines        5691     5695       +4     
  Branches      996      997       +1     
==========================================
  Hits         1922     1922              
- Misses       3189     3192       +3     
- Partials      580      581       +1
Impacted Files Coverage Δ
blocks/color-palette/index.js 0% <0%> (ø) ⬆️

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 e7f1223...a4d10f7. Read the comment docs.

codecov bot commented Sep 6, 2017

Codecov Report

Merging #2680 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2680      +/-   ##
==========================================
- Coverage   33.77%   33.74%   -0.03%     
==========================================
  Files         191      191              
  Lines        5691     5695       +4     
  Branches      996      997       +1     
==========================================
  Hits         1922     1922              
- Misses       3189     3192       +3     
- Partials      580      581       +1
Impacted Files Coverage Δ
blocks/color-palette/index.js 0% <0%> (ø) ⬆️

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 e7f1223...a4d10f7. Read the comment docs.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 6, 2017

Contributor

Screenshots to illustrate the interaction with Safari + VoiceOver:

screen shot 2017-09-06 at 08 54 44

when clicking:

screen shot 2017-09-06 at 08 54 49

when clicking again:

screen shot 2017-09-06 at 08 54 53

selected state:

screen shot 2017-09-06 at 09 01 49

Contributor

afercia commented Sep 6, 2017

Screenshots to illustrate the interaction with Safari + VoiceOver:

screen shot 2017-09-06 at 08 54 44

when clicking:

screen shot 2017-09-06 at 08 54 49

when clicking again:

screen shot 2017-09-06 at 08 54 53

selected state:

screen shot 2017-09-06 at 09 01 49

@aduth

Apologies on the delay in reviewing this one.

Show outdated Hide outdated blocks/color-palette/index.js
Show outdated Hide outdated blocks/color-palette/index.js
Show outdated Hide outdated blocks/color-palette/index.js
Show outdated Hide outdated blocks/color-palette/index.js
@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 29, 2017

Contributor

Updated and rebased. But I'm seeing a new CSS glitch in Safari 11, where the rounded focus style on the color buttons is not equally centered. Investigating.

screen shot 2017-09-29 at 15 54 19

Contributor

afercia commented Sep 29, 2017

Updated and rebased. But I'm seeing a new CSS glitch in Safari 11, where the rounded focus style on the color buttons is not equally centered. Investigating.

screen shot 2017-09-29 at 15 54 19

afercia added some commits Sep 29, 2017

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 29, 2017

Contributor

@aduth any specific reason for this rule:
error A space is required before closing bracket react/jsx-space-before-closing

seems a bit weird to me this actually can make any relevant difference:

<span className="blocks-color-palette__custom-color-gradient"/>
<span className="blocks-color-palette__custom-color-gradient" />
Contributor

afercia commented Sep 29, 2017

@aduth any specific reason for this rule:
error A space is required before closing bracket react/jsx-space-before-closing

seems a bit weird to me this actually can make any relevant difference:

<span className="blocks-color-palette__custom-color-gradient"/>
<span className="blocks-color-palette__custom-color-gradient" />
@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 29, 2017

Contributor

The alignment issue in Safari 11 is solved in 0503875. Any objections to merge?

Contributor

afercia commented Sep 29, 2017

The alignment issue in Safari 11 is solved in 0503875. Any objections to merge?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 29, 2017

Member

@aduth any specific reason for this rule:

Like rules on spacing within parentheses, it's merely a style convention aimed at encouraging consistency.

I would be glad to add these to the coding guidelines document but... the current atmosphere leads me to believe that it would be unwise to be codifying our React usage patterns at this point in time 😬

(Will follow-up with a final review)

Member

aduth commented Sep 29, 2017

@aduth any specific reason for this rule:

Like rules on spacing within parentheses, it's merely a style convention aimed at encouraging consistency.

I would be glad to add these to the coding guidelines document but... the current atmosphere leads me to believe that it would be unwise to be codifying our React usage patterns at this point in time 😬

(Will follow-up with a final review)

@aduth

aduth approved these changes Sep 29, 2017

Removed unused stopPropagation method in a4d10f7. Otherwise LGTM 👍

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 29, 2017

Member

Also, in case you're not already using one and are interested, there are a number of editor integrations to surface ESLint issues before it reaches CI: https://eslint.org/docs/user-guide/integrations

Member

aduth commented Sep 29, 2017

Also, in case you're not already using one and are interested, there are a number of editor integrations to surface ESLint issues before it reaches CI: https://eslint.org/docs/user-guide/integrations

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 29, 2017

Contributor

thanks!

Contributor

afercia commented Sep 29, 2017

thanks!

@afercia afercia merged commit a596ceb into master Sep 29, 2017

3 checks passed

codecov/project 33.74% (-0.03%) compared to e7f1223
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@afercia afercia deleted the update/color-picker-a11y-valid-code branch Sep 29, 2017

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