Skip to content

Conversation

@youknowriad
Copy link
Contributor

closes #2105

This PR adds a new Higher Order Component called "withAssertiveMessages" that adds two props "speakAssertive" and "debouncedSpeakAssertive". The pattern of adding these methods was duplicated across multiple components.

This PR tries to resolve #2105 but I'm not sure about the "Term selected" message.

@youknowriad youknowriad added the [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). label Jul 31, 2017
@youknowriad youknowriad self-assigned this Jul 31, 2017
@youknowriad youknowriad requested a review from afercia July 31, 2017 13:28
@aduth
Copy link
Member

aduth commented Jul 31, 2017

Should this be specific to the assertive tone, or should it be generalized to "speak" (polite and assertive).

@afercia
Copy link
Contributor

afercia commented Jul 31, 2017

or should it be generalized to "speak" (polite and assertive).

From an accessibility perspective, there's the need of both. So far, Gutenberg is mostly using "assertive" to give an immediate audible feedback. In other cases there might be the need to have a "polite" message. The only difference is about timing: assertive will stop screen readers from announcing other things they're "speaking" in that moment, while polite will make them announce the message only after they've finished announcing other stuff.

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.

For the a11y part, please see my comment. Fo the architectural part I'd leave that to people more expert than me with React 🙂

@youknowriad
Copy link
Contributor Author

I made the HoC more generic with props taking a second parameter.

@youknowriad youknowriad force-pushed the add/speak-assertive-hoc branch from ba7aa9e to 99ed929 Compare August 2, 2017 08:15
/**
* WordPress dependencies
*/
import { Component } from 'element';
Copy link
Member

Choose a reason for hiding this comment

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

Note with #2172, these need to be updated to prefix the dependencies with @wordpress/. You will need to perform a rebase against the latest version of master and apply your changes:

git fetch origin
git rebase origin/master

@youknowriad youknowriad force-pushed the add/speak-assertive-hoc branch from 99ed929 to e5bb0aa Compare August 3, 2017 15:58
@youknowriad
Copy link
Contributor Author

If no more concerns, I'll merge this soon.

@codecov
Copy link

codecov bot commented Aug 3, 2017

Codecov Report

Merging #2107 into master will increase coverage by 0.8%.
The diff coverage is 31.25%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #2107     +/-   ##
=========================================
+ Coverage   22.14%   22.94%   +0.8%     
=========================================
  Files         139      143      +4     
  Lines        4322     4532    +210     
  Branches      727      791     +64     
=========================================
+ Hits          957     1040     +83     
- Misses       2841     2925     +84     
- Partials      524      567     +43
Impacted Files Coverage Δ
blocks/url-input/index.js 1.69% <0%> (+0.08%) ⬆️
components/form-token-field/index.js 1.07% <0%> (+0.01%) ⬆️
blocks/editable/format-toolbar/index.js 6.97% <0%> (-0.35%) ⬇️
editor/inserter/menu.js 45.45% <50%> (-0.52%) ⬇️
...omponents/higher-order/with-a11y-messages/index.js 66.66% <66.66%> (ø)
blocks/api/parser.js 96.22% <0%> (-0.75%) ⬇️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
editor/index.js 0% <0%> (ø) ⬆️
editor/inserter/index.js 0% <0%> (ø) ⬆️
blocks/library/paragraph/index.js 47.05% <0%> (ø)
... and 8 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 360a5ad...81c1b4d. Read the comment docs.

this.debouncedSpeak = debounce( this.speak.bind( this ), 500 );
}

speak( message, type = 'assertive' ) {
Copy link
Member

Choose a reason for hiding this comment

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

#2107 (comment) notwithstanding, packages/a11y the default type was chosen as 'polite'

https://github.com/WordPress/packages/blob/2eb0aac/packages/a11y/src/index.js#L31

Inconsistency could be confusing

@youknowriad youknowriad merged commit d861fed into master Aug 4, 2017
@youknowriad youknowriad deleted the add/speak-assertive-hoc branch August 4, 2017 08:16
@mtias
Copy link
Member

mtias commented Aug 4, 2017

I think with "withSpokenMessage" may be more clear here than just A11y.

ceyhun pushed a commit that referenced this pull request Apr 22, 2020
…-to-po2android-script

Add a new androidReplacements function to comply with Android Typography lint rules
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.

Add audible message when links are successfully inserted

5 participants