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 eslint config for a11y #1114

Merged
merged 7 commits into from
Feb 4, 2025
Merged

Create eslint config for a11y #1114

merged 7 commits into from
Feb 4, 2025

Conversation

beaesguerra
Copy link
Member

@beaesguerra beaesguerra commented Feb 3, 2025

Summary:

This work is part of the implementation of ADR#781 Enabling more lint rules for accessibility

In these changes, a separate eslint config file for accessibility is set up since not all repos will need a11y config (see config thread from the ADR for more details). Following the eslint docs for Sharing Multiple Configs, it is consumed in projects by adding the following to the project's eslint config file:

extends: ["@khanacademy/eslint-config/a11y"]

The a11y config file currently enables the aphrodite-add-style-variable-name rule from @khanacademy/eslint-plugin. This is the first step to adding more a11y linting rules so that the naming convention for html elements is more predictable for custom component mapping for eslint-plugin-jsx-a11y (see rule docs for more details).

Next steps:

  • Use this new config in WB, perseus, webapp and address lint errors from the aphrodite-add-style-variable-name rule
  • Update the a11y.js config with the config based on the accessibility linting rules ADR
  • Use the updated config in projects and address existing errors by disabling the rules per line. Teams can address existing errors as they work in the area and new errors can be prevented with these lint rules :)

Issue: FEI-6050

Test plan:

Testing extending the new config locally:

  • Run yarn pack in packages/eslint-config-khan. This will create a .tgz file in the directory
  • In another project (like Wonder Blocks), install the package: ex: yarn add -D -W /Users/beaesguerra/khan/wonder-stuff/packages/eslint-config-khan/khanacademy-eslint-config-v5.0.1.tgz
    • Note: you might need to remove the node_modules first to make sure it installed the correct package. You can check by checking if the file exists at node_modules/@khanacademy/eslint-config/a11y.js in the project
    • Make sure @khanacademy/eslint-plugin is at v3.1.1
  • Add "@khanacademy/eslint-config/a11y" to the extends property in the eslint config file
  • Run yarn lint in the project and restart the ESLint server. It should show aphrodite-add-style-variable-name errors
image image

@beaesguerra beaesguerra self-assigned this Feb 3, 2025
Copy link

changeset-bot bot commented Feb 3, 2025

🦋 Changeset detected

Latest commit: 13ae38d

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

This PR includes changesets to release 1 package
Name Type
@khanacademy/eslint-config Minor

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

Copy link

codecov bot commented Feb 3, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.85%. Comparing base (506ed87) to head (13ae38d).
Report is 3 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1114   +/-   ##
=======================================
  Coverage   99.85%   99.85%           
=======================================
  Files          97       97           
  Lines        1392     1392           
  Branches      351      358    +7     
=======================================
  Hits         1390     1390           
  Misses          1        1           
  Partials        1        1           

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 569b98a...13ae38d. Read the comment docs.

},
settings: {
react: {
version: "detect",
Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the settings for the react version following what we have in webapp: https://github.com/Khan/webapp/blob/81dea05cb7a6534847a43e6b057cef0431f60b8c/services/static/.eslintrc.js#L928-L930

@@ -0,0 +1,27 @@
/* eslint-disable import/no-commonjs */
Copy link
Member Author

Choose a reason for hiding this comment

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

This file is mostly copied from the existing config: https://github.com/Khan/wonder-stuff/blob/main/packages/eslint-config-khan/index.js . I'll leave comments in the PR for what's different!

"import/extensions": [".js", ".jsx", ".ts", ".tsx"],
},
rules: {
"@khanacademy/aphrodite-add-style-variable-name": ERROR,
Copy link
Member Author

Choose a reason for hiding this comment

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

Rules are different from the main eslint config!

…nfig file (`@khanacademy/eslint-config/a11y`). It enables the `@khanacademy/aphrodite-add-style-variable-name` rule.
Copy link
Contributor

github-actions bot commented Feb 3, 2025

Size Change: 0 B

Total Size: 4.63 kB

ℹ️ View Unchanged
Filename Size
packages/wonder-stuff-core/dist/browser/es/index.js 1.85 kB
packages/wonder-stuff-sentry/dist/browser/es/index.js 1.65 kB
packages/wonder-stuff-testing/dist/browser/es/index.js 1.12 kB

compressed-size-action

@beaesguerra beaesguerra marked this pull request as ready for review February 3, 2025 23:36
@khan-actions-bot khan-actions-bot requested a review from a team February 3, 2025 23:36
@khan-actions-bot
Copy link
Contributor

Gerald

Required Reviewers
  • @Khan/frontend-infra for changes to .changeset/lucky-ways-worry.md, packages/eslint-config-khan/a11y.js

Don't want to be involved in this pull request? Comment #removeme and we won't notify you of further changes.

Copy link
Contributor

@kevinb-khan kevinb-khan left a comment

Choose a reason for hiding this comment

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

LGTM. I left a couple of suggestions to tidy up the the config in inline comments. Thanks for creating this new config. I'm excited that our eslint config is becoming more modular. 🎉

Comment on lines 8 to 10
// TODO(csilvers): once we properly use node.js for node
// files, get rid of this next line.
node: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can get rid of this since we won't using React components in node scripts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call, I've removed the unnecessary env settings, thanks for the suggestions!

node: true,
browser: true,
es6: true,
jest: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can get rid of this as well since we're importing jest "globals" from @jest/globals now.

@beaesguerra beaesguerra merged commit 40e273a into main Feb 4, 2025
8 checks passed
@beaesguerra beaesguerra deleted the eslint-a11y-config-khan branch February 4, 2025 17:31
beaesguerra added a commit to Khan/perseus that referenced this pull request Feb 5, 2025
## Summary:
This work is part of the implementation of [ADR#781 Enabling more lint rules for accessibility](https://khanacademy.atlassian.net/wiki/x/IoBVyg)

These changes includes:
- Updated dev dependency: `@khanacademy/eslint-config`
- Extending `@khanacademy/eslint-config/a11y` config in project's eslint config
  - Currently, the a11y config enables the `aphrodite-add-style-variable-name` rule from `@khanacademy/eslint-plugin`. This is the first step to adding more a11y linting rules so that the naming convention for html elements is more predictable for [custom component mapping](https://github.com/jsx-eslint/eslint-plugin-jsx-a11y#component-mapping) for `eslint-plugin-jsx-a11y` (see [rule docs](https://github.com/Khan/wonder-stuff/blob/main/packages/eslint-plugin-khan/docs/aphrodite-add-style-variable-name.md) for more details).
  - `@khanacademy/eslint-config/a11y` will be updated in another PR to include config for `eslint-plugin-jsx-a11y` 
- Addressing lint errors from the aphrodite-add-style-variable-name rule

Issue: FEI-6050

Implementation Plan:
- Khan/wonder-stuff#1114 Set up eslint a11y config and enable aphrodite-add-style-variable-name 
- (includes this PR) Use this new config in WB, perseus, webapp and address lint errors from the aphrodite-add-style-variable-name rule
- Update the a11y.js config with the config based on the accessibility linting rules ADR
- Use the updated config in projects and address existing errors by disabling the rules per line. Teams can address existing errors as they work in the area and new errors can be prevented with these lint rules!

## Test plan:
- Run `yarn lint` and make sure there are no linting errors.
- Some variables were renamed internally, there should be no functional changes

Author: beaesguerra

Reviewers: catandthemachines, #frontend-infra-web

Required Reviewers:

Approved By: catandthemachines

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x)

Pull Request URL: #2193
beaesguerra added a commit to Khan/wonder-blocks that referenced this pull request Feb 6, 2025
## Summary:
This work is part of the implementation of [ADR#781 Enabling more lint rules for accessibility](https://khanacademy.atlassian.net/wiki/x/IoBVyg)

These changes includes:
- Updated dev dependency: `@khanacademy/eslint-config`
- Extending `@khanacademy/eslint-config/a11y` config in project's eslint config
  - Currently, the a11y config enables the `aphrodite-add-style-variable-name` rule from `@khanacademy/eslint-plugin`. This is the first step to adding more a11y linting rules so that the naming convention for html elements is more predictable for [custom component mapping](https://github.com/jsx-eslint/eslint-plugin-jsx-a11y#component-mapping) for `eslint-plugin-jsx-a11y` (see [rule docs](https://github.com/Khan/wonder-stuff/blob/main/packages/eslint-plugin-khan/docs/aphrodite-add-style-variable-name.md) for more details).
  - `@khanacademy/eslint-config/a11y` will be updated in another PR to include config for `eslint-plugin-jsx-a11y` 
- Addressing lint errors from the aphrodite-add-style-variable-name rule

Issue: FEI-6050

Implementation Plan:
- Khan/wonder-stuff#1114 Set up eslint a11y config and enable aphrodite-add-style-variable-name 
- (includes this PR) Use this new config in WB, perseus, webapp and address lint errors from the aphrodite-add-style-variable-name rule
- Update the a11y.js config with the config based on the accessibility linting rules ADR
- Use the updated config in projects and address existing errors by disabling the rules per line. Teams can address existing errors as they work in the area and new errors can be prevented with these lint rules!

## Test plan:
- Run `pnpm lint` and make sure there are no linting errors.
- Some variables were renamed internally, there should be no functional changes

Author: beaesguerra

Reviewers: jandrade

Required Reviewers:

Approved By: jandrade

Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build and test on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️  Chromatic - Skip on Release PR (changesets), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ⏭️  dependabot

Pull Request URL: #2459
beaesguerra added a commit that referenced this pull request Feb 11, 2025
…nfig/a11y` (#1118)

## Summary:
This work is part of the implementation of [ADR#781 Enabling more lint rules for accessibility](https://khanacademy.atlassian.net/wiki/x/IoBVyg)

These changes includes:
- Updating the `eslint-plugin-jsx-a11y` dependency
- Configuring rules and settings (including custom component mapping, attribute mapping, polymorphic components) for `eslint-plugin-jsx-a11y` based on the [proposed rules and settings in the ADR](https://khanacademy.atlassian.net/wiki/spaces/ENG/pages/3394600994/ADR+781+Enabling+more+lint+rules+for+accessibility#Proposed-customization-and-overrides-for-the-plugin) and the [POC](https://github.com/Khan/webapp/pull/26840/files#diff-20e55c78ab86b0eb87c6f756500b87f451b52fab251d6f327ce572ec88e8ae3b)
- Updating the README to include instructions for using `@khanacademy/eslint-config/a11y`

Issue: FEI-1133

Implementation Plan:
- #1114 Set up eslint a11y config and enable aphrodite-add-style-variable-name 
- Use this new config in WB, perseus, webapp and address lint errors from the aphrodite-add-style-variable-name rule
  - Khan/wonder-blocks#2459
  - Khan/perseus#2193
  - Khan/webapp#29212
- (this PR) Update the a11y.js config with the config based on the accessibility linting rules ADR
- Use the updated config in projects and address existing errors by disabling the rules per line. Teams can address existing errors as they work in the area and new errors can be prevented with these lint rules!

## Test plan:
Testing the new config locally:
- Run yarn pack in `packages/eslint-config-khan`. This will create a `.tgz` file in the directory
- In another project, install the package: ex: `yarn add -D -W /Users/beaesguerra/khan/wonder-stuff/packages/eslint-config-khan/khanacademy-eslint-config-v5.1.0.tgz`
  - Note: you might need to remove the node_modules first to make sure it installed the correct package. You can check by checking `node_modules/@khanacademy/eslint-config/a11y.js` in the project to see if the new config is there
- Make sure the project already extends `@khanacademy/eslint-config/a11y` in the project's eslint config file
- Run `yarn lint`/`pnpm lint` in the project and restart the ESLint server. It should show errors from the `jsx-a11y` plugin

Note: Before merging and releasing these changes, I'll test these changes locally with the different projects and evaluate the errors to make sure the config changes are still relevant and helpful!

Author: beaesguerra

Reviewers: beaesguerra, marcysutton, kevinb-khan

Required Reviewers:

Approved By: marcysutton, kevinb-khan

Checks: ✅ codecov/project, ✅ Test (macos-latest, 20.x), ✅ CodeQL, ✅ Lint, typecheck, and coverage check (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ✅ Analyze (javascript), ✅ gerald, ⏭️  dependabot

Pull Request URL: #1118
beaesguerra added a commit to Khan/wonder-blocks that referenced this pull request Feb 12, 2025
## Summary:
This work is part of the implementation of [ADR#781 Enabling more lint rules for accessibility](https://khanacademy.atlassian.net/wiki/x/IoBVyg)

These changes include:
- Updating `@khanacademy/eslint-config` to the latest version to include new jsx-a11y configuration and settings
- Addressed existing errors by adding `eslint-disable-next-line`
  - View the PR commits for a breakdown of errors by rule
  - Used `npx [email protected] .` to easily disable lint errors per rule
- New potential a11y issues can be prevented with these lint rules!

Note: In a follow up PR, I'll address the low hanging issues in WB and create tickets for more complex issues that were found

A breakdown of the existing jsx-a11y errors:

<img width="1027" alt="wb-linting-errors-before" src="https://github.com/user-attachments/assets/cbe4be7f-6868-48af-8474-bad0297cf8b2" />


Implementation Plan:
- Khan/wonder-stuff#1114 Set up eslint a11y config and enable aphrodite-add-style-variable-name 
- Use this new config in WB, perseus, webapp and address lint errors from the aphrodite-add-style-variable-name rule
  - #2459
  - Khan/perseus#2193
  - Khan/webapp#29212
- Khan/wonder-stuff#1118 Update the eslint-config/a11y config with the config based on the accessibility linting rules ADR
- (includes this PR) Use the updated config in projects and address existing errors by disabling the rules per line.

Issue: FEI-1133

## Test plan:
- Run `pnpm lint` and make sure there are no linting errors

Author: beaesguerra

Reviewers: beaesguerra, jandrade

Required Reviewers:

Approved By: jandrade

Checks: ✅ Chromatic - Get results on regular PRs (ubuntu-latest, 20.x), ✅ Lint / Lint (ubuntu-latest, 20.x), ✅ Test / Test (ubuntu-latest, 20.x, 2/2), ✅ Test / Test (ubuntu-latest, 20.x, 1/2), ✅ Check build sizes (ubuntu-latest, 20.x), ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Chromatic - Build and test on regular PRs / chromatic (ubuntu-latest, 20.x), ✅ Prime node_modules cache for primary configuration (ubuntu-latest, 20.x), ⏭️  Chromatic - Skip on Release PR (changesets), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ gerald, ⏭️  dependabot

Pull Request URL: #2471
beaesguerra added a commit to Khan/perseus that referenced this pull request Feb 13, 2025
## Summary:
This work is part of the implementation of [ADR#781 Enabling more lint rules for accessibility](https://khanacademy.atlassian.net/wiki/x/IoBVyg)

These changes include:
- Updating `@khanacademy/eslint-config` to the latest version to include new jsx-a11y configuration and settings
- Updating `esling-plugin-jsx-a11y` to the latest version so it is up to date
- Addressed existing errors by adding `eslint-disable-next-line`
  - View the PR commits for a breakdown of errors by rule
  - Used `npx [email protected] .` to disable lint errors per rule
- Provided custom component mapping for jsx-a11y in the eslint config since Perseus has custom components 

New potential a11y issues can be prevented with these lint rules and existing errors can be addressed over time by the team! The lint error messages have a link to the [rule docs](https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/tree/main/docs/rules) for more details!

A breakdown of the existing jsx-a11y errors:

<img width="1019" alt="perseus-linting-errors-before" src="https://github.com/user-attachments/assets/8191e551-4147-43cb-bbcf-79b0e5684c26" />

### Implementation Plan:
- Khan/wonder-stuff#1114 Set up eslint a11y config and enable aphrodite-add-style-variable-name 
- Use this new config in WB, perseus, webapp and address lint errors from the aphrodite-add-style-variable-name rule
  - Khan/wonder-blocks#2459
  - #2193
  - Khan/webapp#29212
- Khan/wonder-stuff#1118 Update the eslint-config/a11y config with the config based on the accessibility linting rules ADR
- (includes this PR) Use the updated config in projects and address existing errors by disabling the rules per line.

Issue: FEI-1133

## Test plan:
- Run `yarn lint` and make sure there are no linting errors

Author: beaesguerra

Reviewers: catandthemachines, beaesguerra, #frontend-infra-web

Required Reviewers:

Approved By: catandthemachines

Checks: ✅ Publish npm snapshot (ubuntu-latest, 20.x), ✅ Lint, Typecheck, Format, and Test (ubuntu-latest, 20.x), ✅ Cypress (ubuntu-latest, 20.x), ✅ Check for .changeset entries for all changed files (ubuntu-latest, 20.x), ✅ Check builds for changes in size (ubuntu-latest, 20.x), ✅ Publish Storybook to Chromatic (ubuntu-latest, 20.x)

Pull Request URL: #2232
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.

3 participants