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

Update tracking prompt accessibility #2217

Merged
merged 6 commits into from Aug 4, 2017
Merged

Conversation

nylen
Copy link
Member

@nylen nylen commented Aug 4, 2017

Fixes #2215.

  • Remove the default notice dismissal behavior for this notice
  • Move the control buttons in the editor tracking prompt to after the content
  • Move the clarification message behind a "More info" button and a Popover component
  • Add focus styles and any needed aria attributes for the "More info" button

edit: outdated screenshots removed; see below

@nylen nylen added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Status] In Progress Tracking issues with work in progress labels Aug 4, 2017
@nylen nylen requested a review from afercia August 4, 2017 12:00
Put buttons last in markup; move clarification message behind a Popover
@nylen nylen force-pushed the update/tracking-prompt-a11y branch from 2ff367e to 6dedf4c Compare August 4, 2017 12:05
@nylen nylen self-assigned this Aug 4, 2017
@codecov
Copy link

codecov bot commented Aug 4, 2017

Codecov Report

Merging #2217 into master will increase coverage by 0.82%.
The diff coverage is 71.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2217      +/-   ##
==========================================
+ Coverage   22.99%   23.81%   +0.82%     
==========================================
  Files         141      142       +1     
  Lines        4393     4531     +138     
  Branches      746      771      +25     
==========================================
+ Hits         1010     1079      +69     
- Misses       2852     2904      +52     
- Partials      531      548      +17
Impacted Files Coverage Δ
editor/actions.js 37.93% <0%> (-1.36%) ⬇️
editor/index.js 0% <0%> (ø) ⬆️
editor/enable-tracking-prompt/index.js 87.5% <83.33%> (-12.5%) ⬇️
blocks/library/heading/index.js 20.83% <0%> (-2.98%) ⬇️
blocks/library/quote/index.js 13.55% <0%> (-0.73%) ⬇️
blocks/api/paste/strip-attributes.js 0% <0%> (ø) ⬆️
blocks/api/paste/convert-tables.js 0% <0%> (ø)
blocks/api/paste.js 61.53% <0%> (+1.53%) ⬆️
blocks/library/pullquote/index.js 35.71% <0%> (+2.38%) ⬆️
... and 1 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 3fe881a...3601ee3. Read the comment docs.

{ __( 'Usage data is completely anonymous, does not include your post content, and will only be used to improve the editor.' ) }
</Popover>
) }
</Button>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd move the popover out from the button and use aria-expanded on the button based on the showInfoPopover state. See for example post-visibility.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think then we need another container element with position: relative to hold the button and the popover.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, I think that's the reason why a wrapper element was added around button and popover in the post-visibility.

Copy link
Contributor

@afercia afercia left a comment

Choose a reason for hiding this comment

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

Thanks @nylen ! just a coupe small things.

@nylen nylen force-pushed the update/tracking-prompt-a11y branch from e2956fe to 3601ee3 Compare August 4, 2017 14:00
@nylen
Copy link
Member Author

nylen commented Aug 4, 2017

Feedback addressed; focus styles added:

Note that the Popover renders as a div, so it cannot be a child of a p element. Otherwise, the following React warning occurs:

Warning: validateDOMNesting(...): <div> cannot appear as a descendant of <p>.

@nylen nylen removed the [Status] In Progress Tracking issues with work in progress label Aug 4, 2017
@afercia
Copy link
Contributor

afercia commented Aug 4, 2017

Thanks! :shipit:

@nylen nylen merged commit 69c0a6c into master Aug 4, 2017
@nylen nylen deleted the update/tracking-prompt-a11y branch August 4, 2017 15:10
nylen added a commit that referenced this pull request Aug 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants