Skip to content

Update tracking prompt accessibility#2217

Merged
nylen merged 6 commits intomasterfrom
update/tracking-prompt-a11y
Aug 4, 2017
Merged

Update tracking prompt accessibility#2217
nylen merged 6 commits intomasterfrom
update/tracking-prompt-a11y

Conversation

@nylen
Copy link
Copy Markdown
Member

@nylen nylen commented Aug 4, 2017 •
edited
Loading

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
@nylen
Restructure tracking opt-in notice
6dedf4c 
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 mentioned this pull request Aug 4, 2017
Closed
@nylen nylen self-assigned this Aug 4, 2017
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 4, 2017 •
edited
Loading

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.

afercia
afercia reviewed Aug 4, 2017
View reviewed changes
editor/enable-tracking-prompt/index.js Outdated
{ __( 'Usage data is completely anonymous, does not include your post content, and will only be used to improve the editor.' ) }
</Popover>
) }
</Button>
Copy link
Copy Markdown
Contributor

@afercia afercia Aug 4, 2017

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
Copy Markdown
Member Author

@nylen nylen Aug 4, 2017

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
Copy Markdown
Contributor

@afercia afercia Aug 4, 2017

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.

afercia
afercia requested changes Aug 4, 2017
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@nylen
Add a test for tracking prompt's "More info" button
4f98bcf 
See #2217.
@nylen nylen mentioned this pull request Aug 7, 2017
Closed
Tug pushed a commit that referenced this pull request May 12, 2020
@ceyhun
Merge pull request #2217 from wordpress-mobile/update-release-notes
7cc1479
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@afercia afercia afercia approved these changes

Assignees

@nylen nylen

Labels

[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes).

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nylen @afercia