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

Introduce a new rule to flag Link in text blocks without inline #183

Merged
merged 24 commits into from
Jun 5, 2024

Conversation

khiga8
Copy link
Contributor

@khiga8 khiga8 commented May 20, 2024

Relates to:

What

This PR introduces a new accessibility lint rule to ensure that links inside of a text block have the inline prop.

Why

The current recommendation in dotcom is to add inline prop on all Links that appear within a text block to ensure that a user’s accessibility link underline setting is respected.

However, there are currently no automated tools to enforce that developers actually set this prop on their links. As a result, developers continue to add Links to text blocks contributing to 1.4.1: Use of Color accessibility issues.

By having this lint rule in place, we can proactively surface to developers when they should set the inline prop.

Implementation details

The logic essentially treats any Link that appears with a string literal on either side of it as a link within a text block. It checks for the presence of inline or inline={true}.

For example, the following will be flagged:

<p>
  Learn more about <Link href={...}>GitHub</Link>.
</p>
<SomeComponent>
  <Link href={...}>GitHub</Link> is where you share code.
</SomeComponent>

In addition, this lint rule aims to minimize false positives and will skip Links even if adjacent text is detected for the following scenarios:

  • <Link sx={{fontWeight:...}}> or <Link sx={{fontFamily:...}}> - these technically may provide sufficient distinguishing styling.
  • <Link> where the only adjacent text is a period - these can't really be considered a text block.
  • <Link> where the children is a JSX component, rather than a string literal - this is because the JSX component may be an icon link, in which case it may be sufficient to distinguish.
  • <Link> that are nested inside of headings - these feel safer to skip...

Testing / Validation

I've tested this branch against dotcom to make sure there's nothing unexpected! This flagged around 140 Link missing inline in dotcom. I've gone ahead and opened fixes for whatever was flagged by this rule across 8 PRs.

I broke this up into reviewable chunks so that codeowners can make sure to minimize false positives raised by this rule. (Related: https://github.com/github/accessibility/issues/6434)

Reviewers 📣

  1. I am open to any feedback on this logic! Is there anything I'm missing?
  2. Is it okay to enable this rule in recommended.js or is this only relevant to dotcom?
  3. Right now this rule will flag <Link underline> even though technically underline does provide sufficient styling. I decided to flag underline because I noticed it is deprecated so maybe this is an opportunity for consumers to migrate to inline. Let me know if you think we shouldn't flag underline!

Copy link

changeset-bot bot commented May 20, 2024

🦋 Changeset detected

Latest commit: 5a27eea

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-plugin-primer-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@khiga8 khiga8 force-pushed the kh-lint-link-in-text-block branch 2 times, most recently from 9c1e307 to c859fa0 Compare May 28, 2024 21:32
@khiga8 khiga8 force-pushed the kh-lint-link-in-text-block branch from 48aa412 to fbe6078 Compare May 29, 2024 00:38
@khiga8 khiga8 changed the title New rule to flag links in text block New rule to flag links in text block missing inline May 30, 2024
@khiga8
Copy link
Contributor Author

khiga8 commented May 30, 2024

I'm still in the process of getting feedback to make sure there's nothing unexpected with the instances flagged by this rule via PRs in dotcom but want to open this up for early review!

@khiga8 khiga8 marked this pull request as ready for review May 30, 2024 12:45
@khiga8 khiga8 requested a review from a team as a code owner May 30, 2024 12:45
@khiga8 khiga8 requested a review from mperrotti May 30, 2024 12:45
@siddharthkp
Copy link
Member

siddharthkp commented May 30, 2024

Makes sense to me on a theoretical level, hard to review without testing it. Can we run this once on gh/gh to see if it catches any false positives?

Update: Just saw you are already doing that ❤️

@khiga8 khiga8 changed the title New rule to flag links in text block missing inline Introduce a new rule to flag Link in text blocks without inline May 30, 2024
@khiga8
Copy link
Contributor Author

khiga8 commented May 31, 2024

All PRs in dotcom have been reviewed and merged without any issues!
This is ready for a formal review! :D

@@ -16,6 +16,7 @@ module.exports = {
'primer-react/new-color-css-vars': 'error',
'primer-react/a11y-explicit-heading': 'error',
'primer-react/no-deprecated-props': 'warn',
'primer-react/a11y-link-in-text-block': 'error',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that inline is specific to dotcom's setup, maybe this should not be turned on by default. would love to hear what others think!

Copy link
Member

Choose a reason for hiding this comment

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

True, we shouldn't turn it on by default

@khiga8 khiga8 requested a review from dipree June 3, 2024 21:08
@khiga8 khiga8 requested a review from siddharthkp June 4, 2024 13:03
Copy link
Member

@siddharthkp siddharthkp left a comment

Choose a reason for hiding this comment

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

LGTM!

src/configs/recommended.js Outdated Show resolved Hide resolved
@khiga8 khiga8 merged commit 7bd36d2 into main Jun 5, 2024
8 checks passed
@khiga8 khiga8 deleted the kh-lint-link-in-text-block branch June 5, 2024 00:23
@primer-css primer-css mentioned this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants