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

Create a lint rule to encourage inline prop for <Link>s in text block #182

Closed
1 task
khiga8 opened this issue May 20, 2024 · 4 comments
Closed
1 task
Assignees
Labels

Comments

@khiga8
Copy link
Contributor

khiga8 commented May 20, 2024

Problem statement

There is a gap in in-editor tooling to ensure that Links in text block have the inline prop set on it for accessibility.

While we do have the in-browser axe checks to encourage setting inline, having tooling as people are writing code is the most impactful/proactive. The in-browser axe checks are limited in coverage.

Akin to the lint rule that we've introduced in ERB, there is an opportunity to introduce an ESLint rule in Primer React that nudges setting the inline prop for <Link> when the link is identified as being inside of a block. This would also help prepare for the next major release when inline becomes the default.

Important note: Static analysis is limited so this will not flag everything. However, it should help improve coverage!

Acceptance criteria

  • Introduce a new lint rule that flags any links that are detected as being inside of a text block, but missing the inline prop.
@khiga8 khiga8 self-assigned this May 20, 2024
@khiga8 khiga8 changed the title Create a lint rule to encourage setting inline for <Link> identified as being within a text block Create a lint rule to encourage inline prop for <Link>s that are identified as being within a text block May 20, 2024
@khiga8 khiga8 changed the title Create a lint rule to encourage inline prop for <Link>s that are identified as being within a text block Create a lint rule to encourage inline prop for <Link>s in text block May 20, 2024
@broccolinisoup
Copy link
Member

Hello @khiga8 👋 Thanks for raising this issue!

We chatted this in our planning session and have some thoughts about the rule, especially the note you left in the description.

Important note: Static analysis is limited so this will flag everything. However, it should help improve coverage!

If it is going to flag everything, it feels like it can cause friction and get in the way rather than being helpful and it is not ideal. Especially for cases where we need to disable and ignore the rule of the Link components that are not in a text block.

Static analysis is limited

We talked about ways of finding Link components that have text around and although we think 100% coverage is difficult in this case, but we can still try starting small and growing from there. For example, looking at the markup at dotcom and finding the most common "text" components which might be Text, p tags etc and making a list out it to warn against rather than all Link components.

Would love to hear what you think about all these 🙌

@khiga8
Copy link
Contributor Author

khiga8 commented May 30, 2024

Static analysis is limited so this will flag everything.

Ah I'm sorry that was definitely a typo missing a word 🤦‍♀ .

I meant to say: "Static analysis is limited so this will not flag everything." Let me edit that!

I actually have a draft of the lint rule in #183. I've run this against dotcom and it's been able to catch around ~140 instances of Links missing the inline prop. The branch is in draft since I want to verify how it runs against dotcom, but feel free to leave a comment if you have any thoughts.

I agree that false positives should be minimized and have outlined a plan in https://github.com/github/accessibility/issues/6434.

I'm starting out by opening PRs to fix the instances caught by this draft rule which I've gone ahead and broke up into 8 PRs. My plan is to make sure that there are no false positives raised by this rule, iterate on feedback from the code owners, then open up the lint rule for review if we're confident in it having minimal false positives.

Let me know if you have concerns with this!

@broccolinisoup
Copy link
Member

broccolinisoup commented May 30, 2024

No worries and all sounds great!!

Are you happy for me to assign this issue to you since you already have a PR? @khiga8

Didn't realised already assigned to you 😬

Thanks!

@khiga8
Copy link
Contributor Author

khiga8 commented Jun 5, 2024

@khiga8 khiga8 closed this as completed Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants