Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Switch input labels to use <label> element #87

Open
emplums opened this issue Jun 1, 2017 · 6 comments
Open

Switch input labels to use <label> element #87

emplums opened this issue Jun 1, 2017 · 6 comments

Comments

@emplums
Copy link
Contributor

emplums commented Jun 1, 2017

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>

@hharnisc
Copy link
Contributor

I think it's time we create a Label component 😄

@emplums
Copy link
Contributor Author

emplums commented Jun 13, 2017

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

@hharnisc
Copy link
Contributor

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

@hharnisc
Copy link
Contributor

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 😄

@emplums
Copy link
Contributor Author

emplums commented Jun 13, 2017

@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)

@hharnisc
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants