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

JSX-A11Y label-has-for Configuration #7

Merged
merged 2 commits into from Sep 4, 2018

Conversation

Projects
None yet
3 participants
@nancynaluz
Contributor

nancynaluz commented Aug 30, 2018

This PR proposes to make an adjustment to the label-has-for rule under thejsx-a11y linter.

Currently, the configuration of eslint-plugin-jsx-a11y will raise an error if an input is not wrapped with a label and will not recognize an id matched with its corresponding htmlFor as a valid and semantic form input.

This will allow the following to pass:

<label htmlFor="newsletter">Label</label>
<input type="email" id="newsletter" />

Although label-has-for is deemed deprecated, I still find that it's valid for us to raise issues when an input does not have a corresponding label.

nancynaluz
Adjust jsx-a11y linter label-has-for rule.
Allow inputs to have sibling labels with an id associated by htmlFor rather than force labels to wrap around the input.
@nancynaluz

This comment has been minimized.

Contributor

nancynaluz commented Aug 30, 2018

@glossier/front-end CR, please.

nancynaluz
Add spec for label-has-for exception.
Demonstrate example of using htmlFor with id for label and inputs for the label-has-for exception introduced in this commit: eb6e63b.
@LaurentDario

This comment has been minimized.

LaurentDario commented Aug 31, 2018

So I'm questioning myself about the whole logic of label.
We can use aria-label on an input having no Label. In a way, I'd rather have a label everytime we have an input field, but do you think we should consider those times where design will want a simple field with nothing else? The way we're re-doing the fields, anyway, forces us to have a Label as placeholder, but do you think we should still try to cover the rare case we might have of no label?

I don't think we should for now, and if you agree then let's go on with this PR!

@nancynaluz

This comment has been minimized.

Contributor

nancynaluz commented Aug 31, 2018

@LaurentDario Actually I ran this test and it passes:

test('allows stand alone inputs with aria-label', t => {
  const result = lint(
    `const template = <div><input type='checkbox' aria-label='label' /></div>`
  )
  t.is(result.errorCount, 0)
})

What's weird is that this also passes:

test('allows stand alone inputs', t => {
  const result = lint(`const template = <div><input type='checkbox' /></div>`)
  t.is(result.errorCount, 0)
})

But I would think that this would error out if it has no label or aria-label...

I think with the new design system, we'll always have a visible label so I think the proposed set up of using label-has-for should cover our grounds and at least error out inputs that don't have a corresponding id associated to the label.

For example, this would fail:

test('throws error when there is no associated label to the input', t => {
  const result = lint(
    `const template = <div><label>label</label>
  <input id='checkbox' type='checkbox' /></div>`
  )
  t.is(result.errorCount, 1)
})
@simonwalsh

This makes a lot of sense to me 🍗 Thanks for doing this here and not in glossier.com @nancynaluz !

@nancynaluz nancynaluz merged commit 5cfc0fa into master Sep 4, 2018

@nancynaluz nancynaluz deleted the feature/jsx-a11y-linter-label-has-for-fix branch Sep 4, 2018

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