Add aria-describedby prop to Combo Box and Spin Button. Fixes #8045#8057
Add aria-describedby prop to Combo Box and Spin Button. Fixes #8045#8057JasonGore merged 11 commits intomicrosoft:masterfrom
Conversation
|
|
common/changes/office-ui-fabric-react/master_2019-02-20-22-30.json
Outdated
Show resolved
Hide resolved
|
Size Auditor did not detect a change in bundle size for any component! |
|
@JasonGore Can you take another look? Thanks :-) |
|
Can you add some unit tests to make sure the prop is applied correctly? You can search the codebase for similar examples. This will help prevent regressions to the prop you've added. Thanks! |
|
@JasonGore Updated. |
There was a problem hiding this comment.
Very much appreciate the change and the test coverage @Dalimil. I'm blocking for now given the usage of mergeAriaAttributeValues since it could lead to invalid ARIA values. See below:
We need to take a look at mergeAriaAttributeValues given the value for aria-describedby must be an "ID reference list". Does the value need to be space delimited?
https://www.w3.org/WAI/PF/aria-1.1/states_and_properties#aria-describedby
MDN states the value should be:
a space-separated list of element IDs
cc: @natalieethell
|
@KevinTCoughlin Sorry, yes, I forgot the space. I think I may have seen similar usage somewhere else in the codebase so I think that's why I got confused. |
|
Might be a good idea to update the unit test to verify spacing is present too. I need to revisit that utility to deal with spaces or not spaces accordingly and handle both cases. |
|
@JasonGore I updated the unit tests. |
Cool, yea it looks like we have an issue elsewhere in the codebase that we'll address separately (PR linked). For this PR, your approach looks acceptable. I will update this instance if need be with the follow-up PR. Thanks for your patience and updates! |
|
Hrmmm actually the snapshots are failing because now in the default case they'll receive a space as the value for Can you easily guard against that case? If not, I should be able to resolve the underlying issue with |
|
@Dalimil what about this ternary as a workaround to unblock your PR: mergeAriaAttributeValues('one', ' ', 'two')
"one two"
mergeAriaAttributeValues(undefined, ' ', undefined)
" "
let foo = 'foo'
mergeAriaAttributeValues(foo, foo ? ' ' : undefined, 'bar')
"foo bar"
foo = undefined
mergeAriaAttributeValues(foo, foo ? ' ' : undefined, 'bar')
"bar"
mergeAriaAttributeValues(foo, foo ? ' ' : undefined, undefined)
undefinedI can clean it up in the follow-up PR. |
|
@KevinTCoughlin I think that should be fixed now. |
|
@JasonGore Are we ready to merge this now? |
|
I've approved it, Kevin has the block. 😄 |
KevinTCoughlin
left a comment
KevinTCoughlin
left a comment
There was a problem hiding this comment.
minor suggestion re: utilities package change-note. Otherwise LGTM.
We'll still need to fix mergeAriaAttributeValues in a separate PR. Thanks for this contribution and bringing the utility issue to our attention!
|
@JasonGore @KevinTCoughlin The previous build failed because of a long line in a different part of the codebase (someone must have changed something in the meantime). I fixed the tslint issue, so hopefully it's fine now. |
|
@JasonGore @KevinTCoughlin Should be good to go! |
|
🎉 Handy links: |
1 similar comment
|
🎉 Handy links: |
|
🎉 Handy links: |
1 similar comment
|
🎉 Handy links: |
5 participants
Pull request checklist
$ npm run changeDescription of changes
Adds aria-describedby prop to Combo Box and Spin Button.
Microsoft Reviewers: Open in CodeFlow