buffer-components icon indicating copy to clipboard operation
buffer-components copied to clipboard

Switch input labels to use <label> element

Open emplums opened this issue 7 years ago • 6 comments

Heya @hharnisc @alvarezmelissa87 @stevemdixon!

Just a heads up on an axe-core error I'm seeing while adding the tests - it looks like we're using a div element for the input labels instead of a <label>

const renderLabel = ({ label }) => (
  label ? (
    <div style={labelStyle}>
      <Text>{label}</Text>
    </div>
  ) : null
);

I wonder if instead of using a <Text> component for this we could use the native <label> component? That way screen readers have an easier time knowing which label is associated to which input.

Here's an example of how we might do that:

const renderLabel = ({ label }) => (
  label ? (
    <label for={id}>{label}</label>
  ) : null
);

The only thing tricky about this is that I believe we'll need a unique id for each instance of the input in order to associate the label with a particular <input>

emplums avatar Jun 01 '17 16:06 emplums

I think it's time we create a Label component :smile:

hharnisc avatar Jun 13 '17 17:06 hharnisc

@hharnisc agreed :) Do you have any ideas around creating unique id's to pass to the id tag?

emplums avatar Jun 13 '17 17:06 emplums

@emily-plummer taking a quick look at the http://redux-form.com/6.8.0/ docs I think we get some help there :smile:

hharnisc avatar Jun 13 '17 17:06 hharnisc

So with each Field we have to specify a name prop. And that needs to be unique within each form. Each form needs a unique form id.

unique name:

https://github.com/bufferapp/buffer-web-components/blob/master/DateTimeForm/index.jsx#L47

unique form:

https://github.com/bufferapp/buffer-web-components/blob/master/DateTimeForm/index.jsx#L88

I'm expecting it to be easy to grab the name within the component, but not sure about the form id -- seems like it should be possible though. I've got some ideas how to get around this if we can't get the form though.

Does for on the label get read by the screen reader @emily-plummer? Need to understand that a little better before trying to implement this one I think :smile:

hharnisc avatar Jun 13 '17 18:06 hharnisc

@hharnisc I don't think so, the for just lets the screen reader know which form element the label is associated with! An alternative way to associate a label with a form element is to wrap the form element with a label element like so:

  return (
    <div>
      <Label label={label} isHidden={hideLabel}>
        <input
          style={style}
          disabled={meta.submitting}
          value={input.value}
          onChange={input.onChange}
          placeholder={placeholder}
          type={type}
          onFocus={onFocus}
          onBlur={onBlur}
        />
      </Label>
      {renderError(meta)}
    </div>
  );

and then the label component would look like this:

return (
 <label>
   {label}
 </label>
);

That would get around needing to grab an id for each input & would ensure that every input has a label! We can also pass an option hideLabel that can visually hide the label if needed (but still make the label accessible to screen readers) - it would be ideal if all labels were visible but there may be some cases where there's enough context that a visual label might not be necessary (I'm thinking of dropdowns where the first item helps imply meaning like a dropdown of countries or state names)

emplums avatar Jun 13 '17 18:06 emplums

Seems like a great idea to always have a label (visible or otherwise!). Pretty cool that the label can be associated with the input with nesting too!

I think it will keep things simpler if we use the for and id method since they're decoupled and give us some freedom around how to show and hide components.

hharnisc avatar Jun 13 '17 18:06 hharnisc