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

Bump eslint-plugin-jsx-a11y version #2690

Merged
merged 6 commits into from
Jan 12, 2018
Merged

Conversation

gaearon
Copy link
Contributor

@gaearon gaearon commented Jun 29, 2017

This should fix #2663.
Probably a breaking change.

cc @AlmeroSteyn @jessebeach for review (do we need to enable any new rules? disable old ones?)

Copy link
Contributor

@AlmeroSteyn AlmeroSteyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great PR! I was thinking of submitting the same change tomorrow!

Current change makes the rule too restrictive for CRA environment so that probably needs to be changed. I provided some options.

Its a breaking change if anchor-is-valid and/or label-has-for is activated.

Aside from the new anchor-is-valid rule, there has been a fix to label-has-for. I did not add it in the previous PR as it would fail on things like:

<label>Name
<input>
</label>

and

<label>
{children}
</label>

Both of which are accessible patterns.

As this rule checks that each form element has a good chance of having an accessible label it would be an incredible rule to add. I do estimate quite a bit of failures in the wild though.

But the fix would be to add htmlFor to id links. That is easy enough. But in components it could mean that the id need to autogenerated if the component is used more than once on any specific page.

Do you think the benefit is large enough to outweight the impact on the users?

@@ -272,13 +272,13 @@ module.exports = {
'jsx-a11y/accessible-emoji': 'warn',
'jsx-a11y/alt-text': 'warn',
'jsx-a11y/anchor-has-content': 'warn',
'jsx-a11y/anchor-is-valid': 'warn',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The anchor-is-valid rule has quite a bit more reach than the old href-no-hash rule. It provides three checks:

  1. Checks if there is an href on an <a> element.
  2. Checks if href attributes contain obviously valid content.
  3. Checks if and <a> is obviously used as a button.

The third option seems too restrictive for CRA which renders this setup too restrictive.

Options 1 and 2 I think could be usable. The anchors that would fail for the three rules are listed here https://github.com/evcohen/eslint-plugin-jsx-a11y/blob/master/docs/rules/anchor-is-valid.md#fail

The subchecks can be activated as needed.

To activate options 1 and 2 the config is :

"jsx-a11y/anchor-is-valid": [ "warn", {
                    "aspects": [ "noHref", "invalidHref" ]
          }]

Just using the invalidHref aspect gets closest to what was already in CRA although it will error on anchors with javascript in the href.

What do you think? Can we activate both aspects?

@@ -272,13 +272,16 @@ module.exports = {
'jsx-a11y/accessible-emoji': 'warn',
'jsx-a11y/alt-text': 'warn',
'jsx-a11y/anchor-has-content': 'warn',
'jsx-a11y/anchor-is-valid': [
'warn',
aspects: ['noHref', 'invalidHref']

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the wrapping object braces are missing here.

@jessebeach
Copy link

Failing because:

Definition for rule 'jsx-a11y/anchor-is-valid' was not found

Looking into it.

@gaearon
Copy link
Contributor Author

gaearon commented Jun 29, 2017

Maybe some bad CI cache on our side.

@jessebeach
Copy link

Maybe some bad CI cache on our side.

That's what I'm thinking. The rule is there and represented in the index.js.

@jessebeach
Copy link

Maybe some bad CI cache on our side.

No, it's really failing. I cloned from your repo and checked out your branch, ran npm i then

+cd packages/react-error-overlay/
+./node_modules/.bin/eslint --max-warnings 0 src/

and the errors pop up. Investigating.

@jessebeach
Copy link

@gaearon packages/react-error-overlay/ is still on the 5.x.x branch of eslint-plugin-jsx-a11y. Maybe this dependency needs to be removed from this project so that it uses the parent version or version bumped itself?

@vcarel
Copy link

vcarel commented Sep 27, 2017

I believe this PR should fix the Definition for rule 'jsx-a11y/href-no-hash' was not found issue when forcing eslint-plugin-jsx-a11y to 6.0.*.

Please merge it :)

@South-Paw
Copy link

Hi guys,

is there any ETA on when this will be merged?

Thanks 👍

@Timer
Copy link
Contributor

Timer commented Nov 17, 2017

Reminder to update our docs (e.g. #3460).

@gaearon gaearon changed the base branch from master to next January 12, 2018 20:07
@gaearon gaearon merged commit 5579d44 into facebook:next Jan 12, 2018
Timer pushed a commit to Timer/create-react-app that referenced this pull request Jan 13, 2018
* Bump eslint-plugin-jsx-a11y

* Update index.js

* Update index.js

* Update package.json

* Don't use links for non-linky things
gaearon added a commit that referenced this pull request Jan 13, 2018
* Bump eslint-plugin-jsx-a11y

* Update index.js

* Update index.js

* Update package.json

* Don't use links for non-linky things
Timer pushed a commit that referenced this pull request Jan 14, 2018
* Bump eslint-plugin-jsx-a11y

* Update index.js

* Update index.js

* Update package.json

* Don't use links for non-linky things
gaearon added a commit that referenced this pull request Jan 14, 2018
* Bump eslint-plugin-jsx-a11y

* Update index.js

* Update index.js

* Update package.json

* Don't use links for non-linky things
gaearon added a commit that referenced this pull request Jan 14, 2018
* Bump eslint-plugin-jsx-a11y

* Update index.js

* Update index.js

* Update package.json

* Don't use links for non-linky things
gaearon added a commit that referenced this pull request Jan 14, 2018
* Bump eslint-plugin-jsx-a11y

* Update index.js

* Update index.js

* Update package.json

* Don't use links for non-linky things
gaearon added a commit that referenced this pull request Jan 14, 2018
* Bump eslint-plugin-jsx-a11y

* Update index.js

* Update index.js

* Update package.json

* Don't use links for non-linky things
Timer pushed a commit to Timer/create-react-app that referenced this pull request Jan 15, 2018
* Bump eslint-plugin-jsx-a11y

* Update index.js

* Update index.js

* Update package.json

* Don't use links for non-linky things
akstuhl pushed a commit to akstuhl/create-react-app that referenced this pull request Mar 15, 2018
* Bump eslint-plugin-jsx-a11y

* Update index.js

* Update index.js

* Update package.json

* Don't use links for non-linky things
@yutingzhao1991
Copy link

when will release [email protected]?

zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
* Bump eslint-plugin-jsx-a11y

* Update index.js

* Update index.js

* Update package.json

* Don't use links for non-linky things
@lock lock bot locked and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants