Conversation
Lewiscowles1986
force-pushed
the
feature/aria-announce-options
branch
from
6e9102c to
a894960
Compare
lib/render-item.js
Outdated
| const attrValue = valueIsString ? value : value.id; | ||
| const label = valueIsString ? value : value.label; | ||
|
|
||
| item.setAttribute("aria-label", `Choice: ${label}.`); |
There was a problem hiding this comment.
Stopped short of changing this as I felt you may have ideas, opinions, insights into the best place to make Choice: localizable.
Begs the question, should there be an optional localization parameter, config value, that can be stored in state, so server-side request components forward a locale from the initialization of the component?
It feels mildly out of scope for this, but perhaps having that machinery is more important than serving a particularly rigid feature under the guise of accessibility?
There was a problem hiding this comment.
Localisation does feel a bit out of scope here ... perhaps we can tackle it in a separate issue, so we can get this improvement over the finish line?
|
😂 because of the CDN usage when deploying, the changes are not actually visible. |
mroderick
left a comment
mroderick
left a comment
There was a problem hiding this comment.
Would you mind adding some test coverage that asserts that the correct ARIA label is being set?
lib/render-item.js
Outdated
| const attrValue = valueIsString ? value : value.id; | ||
| const label = valueIsString ? value : value.label; | ||
|
|
||
| item.setAttribute("aria-label", `Choice: ${label}.`); |
There was a problem hiding this comment.
Localisation does feel a bit out of scope here ... perhaps we can tackle it in a separate issue, so we can get this improvement over the finish line?
Lewiscowles1986
force-pushed
the
feature/aria-announce-options
branch
from
a894960 to
a998615
Compare
Lewiscowles1986
force-pushed
the
feature/aria-announce-options
branch
2 times, most recently
from
3692cd7 to
38d95b4
Compare
Lewiscowles1986
force-pushed
the
feature/aria-announce-options
branch
from
38d95b4 to
6118908
Compare
Codecov Report
@@ Coverage Diff @@
## master #78 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 27 27
Lines 239 242 +3
=========================================
+ Hits 239 242 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Lewiscowles1986
force-pushed
the
feature/aria-announce-options
branch
from
6118908 to
f19cc50
Compare
|
Hmm.. Says 100% coverage. Did I miss something the automated checkers missed? |
3 participants
Purpose (TL;DR)
To enable screen reader software and assistive ARIA aware technologies to announce each item as highlighted prior to selection.
Fixes #77
Background (Problem in detail)
Detailed in #77
Solution
Uses additional ARIA attributes to announce choices prior to selection, differentiating from selection announcement.
Supporting DOM changes. (likely "choice" constant will need to be configurable).
Other solutions considered
Please see ticket.
How to verify - mandatory
npm cinpm run startContributor declaration