Skip to content

Update eslint-plugin-primer-react #67

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

Merged
merged 19 commits into from
Jul 25, 2023
Merged

Conversation

TylerJDev
Copy link
Member

@TylerJDev TylerJDev commented Jul 13, 2023

This PR provides the following updates:

  • Updates eslint-plugin-jsx-a11y and eslint-plugin-github
  • Removes github mapping
  • Updates jsx-a11y component mapping
  • Fixes bug in a11y-explicit-heading when using spread props, or variable for as
  • Adds docs option to a11y-explicit-heading

@changeset-bot
Copy link

changeset-bot bot commented Jul 13, 2023

🦋 Changeset detected

Latest commit: 2572099

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 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

@@ -15,10 +15,12 @@ const isUsingAsProp = elem => {

const isValidAsUsage = value => validHeadings.includes(value.toLowerCase())
const isInvalid = elem => {
if (elem.attributes.length === 1 && elem.attributes[0].type === 'JSXSpreadAttribute') return;
Copy link
Member Author

Choose a reason for hiding this comment

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

I noticed a bug when <Heading> was used with spread props ({...args}), or when passing a variable withas (<Heading as={as}>).

If the spread is the only attribute present on the component, the checks will skip it. Similarly, if the component is using a variable for the as prop, checks will also skip it.

Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch!!

Button: 'button',
IconButton: 'button'
}
components: require('./components')
Copy link
Member Author

@TylerJDev TylerJDev Jul 18, 2023

Choose a reason for hiding this comment

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

I created an individual file for component mapping (components.js). This allows us to consolidate all the mappings we want in one place. It also introduces a custom structure that is currently a proof of concept which should allow us to easily map composable components. The alternative would be adding the object back into recommended.js instead of separating the two.

Would love additional 👀 on this!

@TylerJDev TylerJDev marked this pull request as ready for review July 18, 2023 14:58
const path = require('path')

module.exports = ({id}) => {
const url = new URL(homepage)
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this handy tool from eslint-plugin-github that resolves the url path for documentation. This will allow us to use the docs option easily.

@TylerJDev TylerJDev requested review from a team, josepmartins and khiga8 July 18, 2023 20:09
Copy link
Contributor

@khiga8 khiga8 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 nicely coming together!!! Good catch with the bug!!

If you haven't already, I'd recommend running this against a dotcom branch -- I think we'll want to especially make sure that none of the component mappings result in false positives being thrown.

@@ -15,10 +15,12 @@ const isUsingAsProp = elem => {

const isValidAsUsage = value => validHeadings.includes(value.toLowerCase())
const isInvalid = elem => {
if (elem.attributes.length === 1 && elem.attributes[0].type === 'JSXSpreadAttribute') return;
Copy link
Contributor

Choose a reason for hiding this comment

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

nice catch!!

Copy link
Contributor

@khiga8 khiga8 left a comment

Choose a reason for hiding this comment

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

One minor suggestion, but otherwise LGTM!

I recommend making sure nothing unexpected is flagged by pointing a dotcom branch against these new updates in eslint-plugin-primer-react.

@TylerJDev
Copy link
Member Author

I created a draft PR in dotcom that shows this branch working as expected!

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