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.

Sign up for GitHub

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

Jump to bottom

[core] Pin eslint-plugin-jsx-a11y version to 6.7.1 #43292

Merged
merged 8 commits into from
Aug 16, 2024
Merged

[core] Pin eslint-plugin-jsx-a11y version to 6.7.1 #43292

merged 8 commits into from
Aug 16, 2024

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Aug 14, 2024

Isolate some of the breaking changes introduced in #43135

@Janpot Janpot added the scope: code-infra Specific to the core-infra product label Aug 14, 2024
@Janpot Janpot changed the title [core] Update eslint-plugin-jsx-a11y [core] Update eslint-plugin-jsx-a11y Aug 14, 2024
@mui-bot
Copy link

mui-bot commented Aug 14, 2024 •
edited
Loading

Netlify deploy preview

https://deploy-preview-43292--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against b387a72

@Janpot Janpot marked this pull request as ready for review August 14, 2024 09:29
ZeeshanTamboli
ZeeshanTamboli reviewed Aug 14, 2024
View reviewed changes
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This library was about to be updated in #39752, but blocked by an issue mentioned here. See the issue: jsx-eslint/eslint-plugin-jsx-a11y#959. Shouldn't the rule only apply to interactive elements, as per the docs? In that case, shouldn't it not fail for the th table header element?

@Janpot
Copy link
Member Author

Janpot commented Aug 14, 2024 •
edited
Loading

You are right, I acted too fast, thanks for catching it. Perhaps we can add th to the ignore list instead?

@ZeeshanTamboli
Copy link
Member

Perhaps we can add th to the ignore list instead?

Shouldn't th be ignored by ESLint by default? It's fine with me, but only if the package update is critical.

@Janpot
Copy link
Member Author

Janpot commented Aug 14, 2024 •
edited
Loading

No, it's not critical, I'll pin the dependency version so that we can proceed with the lockfile maintenance PR.

@ZeeshanTamboli ZeeshanTamboli changed the title [core] Update eslint-plugin-jsx-a11y [core] Pin eslint-plugin-jsx-a11y to 6.7 Aug 16, 2024
@ZeeshanTamboli ZeeshanTamboli changed the title [core] Pin eslint-plugin-jsx-a11y to 6.7 [core] Pin eslint-plugin-jsx-a11y version to 6.7.1 Aug 16, 2024
ZeeshanTamboli
ZeeshanTamboli approved these changes Aug 16, 2024
View reviewed changes
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a comment

Choose a reason for hiding this comment

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

Looks good. Updated the PR title.

@Janpot Janpot merged commit e10e111 into mui:next Aug 16, 2024
22 checks passed
@oliviertassinari oliviertassinari added the scope: docs-infra Specific to the docs-infra product label Sep 8, 2024
@oliviertassinari oliviertassinari mentioned this pull request Sep 8, 2024
Merged
@Msolmaz4 Msolmaz4 mentioned this pull request Sep 17, 2024
Open
@prathik2401 prathik2401 mentioned this pull request Sep 18, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZeeshanTamboli ZeeshanTamboli ZeeshanTamboli approved these changes

Assignees
No one assigned
Labels
scope: code-infra Specific to the core-infra product scope: docs-infra Specific to the docs-infra product
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Janpot @mui-bot @ZeeshanTamboli @oliviertassinari