primer/eslint-plugin-primer-react

ESLint rules for Primer React

eslint-plugin-primer-react

ESLint rules for Primer React

Installation

1. Assuming you already have ESLint and Primer React installed, run:

   npm install --save-dev eslint-plugin-primer-react
   
   # or
   
   yarn add --dev eslint-plugin-primer-react

2. In your ESLint configuration file, extend the recommended Primer React ESLint config:

   {
     "extends": [
       // ...
       "plugin:primer-react/recommended"
     ]
   }

Rules

• direct-slot-children
• no-system-props
• new-css-color-vars
• no-deprecated-props
• a11y-tooltip-interactive-trigger
• a11y-explicit-heading
• a11y-link-in-text-block
• a11y-remove-disable-tooltip
• a11y-use-next-tooltip

eslint-plugin-primer-react's Issues

Create a lint rule that enforces appropriate usage of `Box`

Problem statement

Primer React provides a couple of low-level utility components such as <Box>, <Text>, and <Heading>.

In particular, <Box> seems to be used in all sorts of ways, outside of what the doc describes:

basic wrapper component for most layout related needs.

For example, there's <Box as="h1">, <Box as="button">, when people could be rendering <Heading> or <h1> , or <Button> instead. Box usage outside of "wrapper-y" elements (e.g. div, section) feel like code smells.

There is an opportunity to create a linter that suggests a more appropriate element or component based on the as prop.

This isn't directly tied to accessibility, but components being used in a predictable manner is important.

<Box> rendered without sx are unnecessarily verbose and might as well just be semantic HTML elements. This will not only improve readability, but it would also apparently improve performance.

Related slack thread

Acceptance criteria

• Propose a lint rule that encourages appropriate usage of Box and/or auto-fixes it.
• Incorporate auto-fix.

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

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.

Create a lint rule for disabled controls

Summary

We often have to deal with disabled controls in Primer which are typically inaccessible to most users. If we could create a rule that would help stem that usage, then it might reduce the amount of inaccessible patterns that are commonly used with the disabled attribute.

Idea

We cannot restrict usage of the disabled attribute outright, as it may be a good option in some cases. Since there are many different nuances for how and when this attribute is used, we should restrict this rule to only be applied in specific cases.

In forms

We provide guidance against disabling save/submit buttons in forms and instead provide a reason as to why the submission isn't successful. We could potentially create a rule that throws a violation against disabled submit buttons used within a <form>, or similar form-based components.

Example of bad usage:

<form>
  <label>Name
    <input type="text" name="name" required />
  </label>

  <label>
    Bio
    <textarea name="bio" rows="5" />
  </label>
  
  <!-- The control is disabled, but provides no explanation as to why -->
  <button type="submit" disabled>Save</button>
</form>

We could warn in instances where disabled is used for the submit control, much like in the example above.

As suggestions

ESLint allows you to provide suggestions to patterns without throwing an error. We could utilize this to steer developers away from disabled, by encouraging them to look into using a more accessible pattern that will fits their needs.

Test issue

Create a lint rule for `TriangleDownIcon` inside `trailingAction`

There are scenarios where folks may use TriangleDownIcon inside of trailingVisual instead of trailingAction. This rule should lint against usage in trailingVisual and recommend/auto-fix the usage to be trailingAction. This is for the Button component.

`a11y-link-in-text-block` rule should avoid flagging Links with `className`

Context

The a11y-link-in-text-block rule isn't accounting for how a Link may already have sufficient styling applied through a class.

For example:

<Link href="#" className='some-special-class'>@khiga8</Link> posted in discussion.

It is not possible for the lint rule to know what styles are being applied via some-class. If some-class applies bold styling, then technically speaking, it doesn't need an inline prop.

We may want to skip Link that have className entirely to reduce potential false positives.

Create an eslint rule that suggest using the new accessible Tooltip

Based on our team strategy of rolling out breaking accessibility remediations, we need a supporting eslint rule for Tooltip. The rule will suggest using the new accessible tooltip rather than the one in the main bundle.

From

import { Tooltip } from '@primer/react'

to

import { Tooltip } from '@primer/react/next'

Add support for TypeScript parser to ESLint rule tester

Currently, when testing rules the ESLint parser does not support TypeScript syntax. It'd be great if we could support a TypeScript parser in these tests so that we can author input and output examples in TypeScript.

Create a lint rule that restricts `aria-label` or `aria-labelledby` usage based on tag

We should never allow aria-label or aria-labelledby on roles that do not support it. I've noticed instances of <Text aria-label> and <Box aria-label> in the codebase, which should be flagged.

I think we could flag inappropriate aria-label misuse based on what the as is set as, the role, and the default tag for the component.

Examples of what should be flagged:

<Heading aria-label="Some text">
<Box as="p" aria-label="Some text">
<Text aria-labelledby="some-id">

eslint-plugin-jsx-a11y has limited support for linting (polymorphic) components so unfortunately we aren't getting the a11y linting we need. We could get around this with a custom rule in this plugin.

Error running ESLint due to conflict with `eslint-plugin-primer-react` and `primer/primitives`

Hi! The latest major version of primer/primitives, 8.1.0, appears to remove the deprecated colors folder which this library relies on. As a result, when a project is on the latest version of primer/primitives and we try to run ESLint, we run into:

Oops! Something went wrong! :(

ESLint: 8.57.0

Error: Failed to load plugin 'primer-react' declared in '--config': Cannot find module '@primer/primitives/dist/deprecated/colors'
Require stack:
- /home/runner/work/code-stats-collector/code-stats-collector/node_modules/eslint-plugin-primer-react/src/rules/no-deprecated-colors.js
- /home/runner/work/code-stats-collector/code-stats-collector/node_modules/eslint-plugin-primer-react/src/index.js
- /home/runner/work/code-stats-collector/code-stats-collector/node_modules/@eslint/eslintrc/dist/eslintrc.cjs

Wanted to flag to see if you have a recommendation forward.

It looks like this surfaced as a conflict in - #167.

Add lint rules for recommended imports and prop usage

It'd be great to have two specific rules that we can add to over time:

• use-preferred-imports
• use-preferred-props

These rules would encapsulate our preferred way to import components and prop usage for a component. This could help with migrating components across entrypoints or prop renames.

Create a rule to disallow directly accessing themes from `sx` or styled system props

A no-theme-access-out-of-context rule would keep you from doing this:

import {theme, Box} from '@primer/components'

<Box width={theme.sizes.large} />

And auto-fix it to:

<Box width="large" />

Create an lint rule to warn deprecated component usage

To increase the adoption of our accessible/improved components, we want to add a lint rule that warns consumers if they stay using the deprecated version of the components.

We want to move away from

import { Tooltip } from '@primer/react/deprecated'

to

import { Tooltip } from '@primer/react/experimental'

Create a lint rule that restricts tag usage

Some components allow a as prop to be set. This flexibility can lead to non-semantic component usage which is very detrimental to accessibility.

For example, a <Heading> component should only ever render with a heading tag. However, it can currently be rendered with any tag like <p>.

Until a system-level update is made to define an allowlist for each component that accepts prop, we should restrict as values with a lint rule.

If we can make a system-level change, that would be preferable, but I'm not sure what work is involved for that.

Related Slack convo GitHub staff-only

Update the `a11y-tooltip-interactive-trigger` to exclude elements that have `disabled` attribute from the interactive element list

Add a check to the tooltip interactive trigger rule to exclude elements that have disabled attribute (reference)

Slack ref (GitHub stuff only link)

Make sure `recommended.js` is up-to-date and dependecies are up-to-date

The component to tag mappings in recommended.js have not been updated for a while, and is overdue for a review.

In addition, this library is behind on jsx-a11y and eslint-plugin-github dependency updates.

We have a mapping that is used in eslint-plugin-jsx-a11y and a mapping that is used in eslint-plugin-github. The latter should be deprecated since it was an experimental feature that is deprecated in the latest eslint-plugin-github.

• Are there any component to tag mappings that should be newly added? (Notably, jsx-a11y supports 1:1 component to tag mapping). However, we should be wary of adding some mappings that could raise a false positive. For example, if we map Avatar to img, but it already sets alt internally default, the ESLint rule may incorrectly flag that there's a missing alt.
• Are there any component to tag mappings that should be removed?

Acceptance criteria

• Bump eslint-plugin-github and eslint-plugin-jsx-a11y dependency to the latest
• Ensure that the configs are up-to-date.

[Story] Ensure the `<Box aria-label>` is flagged as inaccessible markup

Context

I noticed there is a gap in flagging aria-label/aria-labelledby misuse on the Box component!

I noticed that the following markup will be flagged by the a11y-role-supports-aria-props rule:

<Box as="div" aria-label></Box>
<Box as="span" aria-labelledby></Box>

This is because we have the polymorphicPropName config set to as, so the linter knows how to map the component to an element.

However, the following markup will not be flagged:

<Box aria-label></Box>
<Box aria-labelledby></Box>

It looks like we can expand coverage and ensure this Box is interpreted as a div by mapping Box to div in the component mapping for github!

Acceptance criteria

• Ensure that invalid ARIA markup on Box is flagged.

Rule no-system-props does not detect ``Parent.Children`` component tags

Example:

             <Header>
                <Header.Item>
                     // Rule does not detect the fontSize
                    <Header.Link href="#" fontSize={2}>
                        // Rule detects the mr
                        <StyledOcticon icon={MarkGithubIcon} size={32} mr={2} />
                        <span>GitHub</span>
                    </Header.Link>
                </Header.Item>
            </Header>

[Idea] Opinionated rule that requires `as` to be set for `<Heading>`

Currently <Heading> defaults to h3 so consumers don't need to explicitly set as.

Default example:

<Heading>Some heading</Heading>

Should heading as always be set to ensure consumers are intentional, rather than obscuring by default?

Here is an example with the same default heading but with level explicitly set:

<Heading as="h3">Some heading</Heading>

cc: @TylerJDev

Error upgrading past v4.0.3 - eslint-plugin-github should be dependency

Hello! Attempting to update past v4.0.3 results in an error when we run our linter saying eslint-plugin-github couldn't be found. It also points to primer-react/recommended as the spot that's referencing it.

It looks like eslint-plugin-github used to be a dependency AND a devDependency, but was removed from the dependencies in #106.

Since you are using eslint-plugin-github in the published primer plugin, I think it should be a dependency and not a devDependency.

I'm not familiar with the codebase though, so there may be a better explanation. Another option might be to make it a peerDependency.