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

[FX-2141] Installs and configures eslint-plugin-styled-components-a11y #6064

Merged
merged 1 commit into from
Aug 20, 2020

Conversation

dzucconi
Copy link
Member

@dzucconi dzucconi commented Aug 6, 2020

Re: FX-2141

I'd like to install a set of linting rules to pick up some basic problems which seem pretty pervasive throughout the site. Hopefully we can lean on the linting to guide developers into building more accessible components.

So we can't use eslint-plugin-jsx-a11y because of styled-components. However eslint-plugin-styled-components-a11y exists and seems to work pretty alright!

Running this against src comes up with 82 errors (I think only so few because of the palette components not being checked, see below) — mostly misuse of onClick handlers.


I might have to turn some rules to warn? Or is it just the case that the linter only runs on commit/push?


One caveat here is that Boxand other palette components aren't being checked, which will be a very big offender. I believe this is due to the weirdness with the primitives.View thing. Though I think we should be able to just split that off into an .ios for now and use a real div. I'll take a look when I install this same plugin in palette.

@damassi
Copy link
Member

damassi commented Aug 6, 2020

What do you think about disabling the keyboard listener rule for now? That will certainly confuse folks without concrete direction.

@dzucconi
Copy link
Member Author

dzucconi commented Aug 6, 2020

@damassi I don't really think any of them should be disabled — they are easy to fix and definitely should be fixed. (Use Clickable, basically). I suspect the problem will just be editing old files which might be problematic (in which case just disabling the rule selectively is better)

@dzucconi
Copy link
Member Author

dzucconi commented Aug 6, 2020

It's probably more a question of how to guide when that comes up... is there a way to pop up a custom warning message?

@damassi
Copy link
Member

damassi commented Aug 6, 2020

Use Clickable, basically

Oh I see -- I had misinterpreted what the linter was guiding the user towards.

is there a way to pop up a custom warning message

Great question / feature.

@dzucconi
Copy link
Member Author

dzucconi commented Aug 6, 2020

@damassi we could start by enabling this in palette first since it's a smaller/stricter surface?

Copy link
Member

@damassi damassi left a comment

Choose a reason for hiding this comment

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

This is a great addition 👍

If there's some lifting / learning that the broader team will need to take on be sure to guide folks in #dev, otherwise LGTM! Merge when 🍏

@damassi
Copy link
Member

damassi commented Aug 6, 2020

we could start by enabling this in palette first since it's a smaller/stricter surface?

Not necessarily -- if the errors are pretty straight forward in Force we should just fix em.

@dzucconi
Copy link
Member Author

dzucconi commented Aug 6, 2020

Cool 👍 will merge on Tuesday when I get back (OOO)

Copy link
Member

@starsirius starsirius left a comment

Choose a reason for hiding this comment

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

Excited to make the site more accessible! I'd suggest fixing a few errors as examples (can be in a followup PR), and we can use them as guidance and point people to the PR.

@sweir27
Copy link
Contributor

sweir27 commented Aug 19, 2020

👋 is this good to go? (once merge conflict is fixed 😄 )

Maybe we could use FE office hours to communicate to people what the changes they should expect are?

@dzucconi
Copy link
Member Author

OK — just rebased and will merge on green

@artsy-peril artsy-peril bot merged commit d3dab7c into artsy:master Aug 20, 2020
@artsy-peril artsy-peril bot mentioned this pull request Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants