improvement: associating label with input

[lloydaf]

What:

​This PR associates the label in the SelectList component with the input element that it contains
Why:

​This is to improve accessibility, before this change, there was a label and there was a select dropdown, but both were not tied to each other
How:

​1. Generate an ID property
2. Assign that to the dropdown
3. Use that id with htmlFor for the label
Media:

https://www.loom.com/share/736830f773bb4343a8ee2232d6e067df
Checklist:

• Ready to be merged
  

[duranmla]

Great! thanks for this @lloydaf what wasn't clear for me is that I can still provide a custom id for my component and it will be bound to the label isn't?

[duranmla]

@lloydaf thanks!
I see a slightly different usage here https://github.com/freenowtech/wave/pull/86/files that allows to devs to pass their own id and whenever it is not present then wave will generate one. It seems to not be the case for this implementation, or I am missing something?

[lloydaf]

As of now (according to this PR), it is auto generated, as the purpose is only to link the Label in the SelectList with the input field.

If we want to make this configurable as a prop, imo we should make the Label configurable as well, because otherwise, if you pass id as a prop and have a label outside SelectList referring to that id, you will end up with two labels for the input field (one from outside and one from inside SelectList), which I think can potentially cause weird accessibility issues. (I could be wrong, maybe the intention is to have multiple labels)

Just to keep in mind, don't know if I missed something.

Do you want to make id configurable as a prop?

[duranmla]

We need the id to be configurable as a prop but we can't have the odd behavior you describe above because indeed is wrong. What I am expecting is to:

<SelectList
  id="select-list-playground"
  label="City"
  onChange={change => console.log(change)}
  options={[
    {
      label: 'Barcelona',
      value: 'bcn',
    },
    {
      label: 'Hamburg',
      value: 'ham',
    },
    {
      label: 'Paris',
      value: 'par',
      isDisabled: true,
    },
  ]}
/>

This should render a select with a label "City" that is associated to the input. Isn't? Maybe I am missing something.

[lloydaf]

I get the intention, but here's what I see could be a problem:

<SelectList
  id="select-list-playground"
  label="City"
  onChange={change => console.log(change)}
  options={[
    {
      label: 'Barcelona',
      value: 'bcn',
    },
    {
      label: 'Hamburg',
      value: 'ham',
    },
    {
      label: 'Paris',
      value: 'par',
      isDisabled: true,
    },
  ]}
/>
<label htmlFor="select-list-playground">Second Label for the city</label>

consider this usecase. If we generate a new id inside SelectList, then this will work fine. The second label will refer to the SelectList component. If we change this to a prop, and pass that prop as the id of the Input, then the second label will refer to the Input. Which I think could be problematic. So imo the current implementation works fine, unless you have a strong opinion about making id configurable, if you can think of a usecase

[duranmla]

I would like to hear @nlopin or @snapsnapturtle in this matter as I am a bit lost with the component. But thanks for outline more context @lloydaf ! 🚀

[nlopin]

@lloydaf the example above is not a valid HTML. According to the HTML spec:

The LABEL element may be used to attach information to controls. Each LABEL element is associated with exactly one form control.

The id prop should be available to be set by a developer in case it is needed, if not provided, we generate the id automatically

[lloydaf]

@nlopin in the next paragraph, it says

More than one LABEL may be associated with the same control by creating multiple references via the for attribute.

This was the usecase I was trying to show above.

As a user (developer), it is not clear to me that the "id" that is assigned to the SelectList component is used to assign to the Input field. Semantically it would make more sense to me if we use inputId as the propname the same way react-select does. Also it still seems to be syntactically possible to have multiple labels point to the same control, according to the spec.

[lloydaf]

Made an inputId prop to pass down to SelectList, updated documentation

[nlopin]

Great job! Sorry that it took a lot of time from our side

[github-actions]

🎉 This PR is included in version 1.5.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀