feat(Label): integrate with Dropdown#1446
Conversation
silviuaavram
requested review from
dvdzkwsk,
kuzhelov,
layershifter,
levithomason,
miroslavstastny and
mnajdova
as code owners
Codecov Report
@@ Coverage Diff @@
## master #1446 +/- ##
==========================================
+ Coverage 73.47% 73.47% +<.01%
==========================================
Files 807 807
Lines 6081 6082 +1
Branches 1768 1750 -18
==========================================
+ Hits 4468 4469 +1
Misses 1608 1608
Partials 5 5
Continue to review full report at Codecov.
|
docs/src/examples/components/Form/Types/FormExampleDropdown.tsx
Outdated
Show resolved
Hide resolved
silviuaavram
added
accessibility
| */ | ||
| itemToString?: (item: ShorthandValue) => string | ||
|
|
||
| labelId?: string |
There was a problem hiding this comment.
do we need this prop? can't we just use 'aria-labelledby' attribute directly?
It also misses documentation
There was a problem hiding this comment.
Oups, will add documentation!
Maybe because we need to compute aria-labelledby from that (in case of the trigger button, we also add its own id). Also because it is used in two places: button/input and ul. I think it's better this way. We will have it computed in the behavior in the future. Will ask for more opinions on this.
There was a problem hiding this comment.
ok, let's see what others say
@layershifter @kuzhelov @mnajdova
…t-ui/react into feat-dropdown-form-field
| label: { content: `Your best friend's name is:`, id: labelId }, | ||
| name: 'chooseFriend', | ||
| key: 'choose-friend', | ||
| required: true, |
There was a problem hiding this comment.
Please remove it, it passes required to div:
<div class="ui-dropdown ui-box" required="" name="chooseFriend" />| const labelId = 'choose-friend-label' | ||
| const fields = [ | ||
| { | ||
| label: { content: `Your best friend's name is:`, id: labelId }, |
There was a problem hiding this comment.
I am surprised with generated markup, is it valid? I expect:
<label for="favoritefood">Favorite food</label> <!-- <<== `for` is used -->
<select name="favoritefood" id="favoritefood">
<option>Cheese</option>
</select>We have:
<label class="ui-text" dir="auto" id="choose-friend-label">Your best friend's name is:</label>
<div class="ui-dropdown ui-box" required="" name="chooseFriend" />And if we will pass id on root:
<label class="ui-text" dir="auto" for="choose-friend-label" id="choose-friend-label">There was a problem hiding this comment.
About label & htmlFor: https://codesandbox.io/s/dark-microservice-47wft
layershifter
left a comment
layershifter
left a comment
There was a problem hiding this comment.
I don't have better ideas than labelId right now. Please also update PR description: it integrates Dropdown and Form.Field.
…t-ui/react into feat-dropdown-form-field
| this.handleTriggerButtonKeyDown(e, rtl) | ||
| }, | ||
| 'aria-label': content, | ||
| 'aria-label': undefined, |
There was a problem hiding this comment.
if somebody provides aria-label should not we apply it to the trigger or to the input as well? the same for aria-describedby
There was a problem hiding this comment.
I don't see it as being suggested by ARIA, and if the user wants to do his own magic with label and describedby he can always use the slots.
But if you think we should handle it by default then we can create a new task.
5 participants
Replacing #1372 with this approach.
Since both the triggerButton/searchInput need to be labelled, I passed the id of the label to the Dropdown via
labelIdand handled it there.Added example of usage.