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

Provide no-redundant-roles exception for rowgroup #531

Merged
merged 1 commit into from
Jun 4, 2024

Conversation

khiga8
Copy link
Contributor

@khiga8 khiga8 commented Jun 4, 2024

Relates to: https://github.com/github/accessibility/issues/5304

Follow-up to: https://github.com/github/accessibility/discussions/4921

What

This PR adds an exception so that the no-redundant-roles rule does not flag redundant use of rowgroup for:

  • <thead role="rowgroup">
  • <tbody role="rowgroup">

Why

There have been 3 reported instances of false positives raised by this rule. All of these relate to redundant usage of role="rowgroup". Redundant usage of role="rowgroup" is necessary in some cases to ensure that table semantics are not dropped by screen readers.

The reported instances are:

It seems appropriate to not flag redundant use of role="rowgroup" for now. We can always revisit this in the future.

Why don't we turn this rule off completely?

All the reported false positives are for role="rowgroup" which we can except, so it does not seem worth turning the rule off completely.

@khiga8 khiga8 requested review from TylerJDev, a team and accessibility-bot June 4, 2024 18:18
@khiga8 khiga8 requested a review from a team as a code owner June 4, 2024 18:18
@khiga8 khiga8 requested a review from mattcosta7 June 4, 2024 18:18
@accessibility-bot
Copy link

👋 Hello and thanks for pinging us! You've entered our first responder queue. An accessibility first responder will review this soon.

  • 💻 On PRs for our review: please provide a review environment with steps to validate, screenshots (with alt text), or videos demonstrating functionality we should be checking. This will help speed up our review and feedback cycle.
  • ⚠️ If this is urgent, please visit us in #accessibility on Slack and tag the first responder(s) listed in the channel topic.

Copy link

@TylerJDev TylerJDev 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, thanks a ton too! ✨

'jsx-a11y/no-redundant-roles': [
'error',
{
nav: ['navigation'], // default in eslint-plugin-jsx-a11y

Choose a reason for hiding this comment

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

Just curious, is this needed because the object is overridden when we provide our own config/mapping? 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is correct!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or at least thats how I remember it.. let me double check 👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I'm interpreting the logic correctly, it looks like the rule will use the default { nav: ['navigation']} if the consumer isn't already passing an alternate option! This seems fine to keep!

@khiga8 khiga8 merged commit 8d9ed4f into main Jun 4, 2024
4 checks passed
@khiga8 khiga8 deleted the kh-set-exception-on-rule branch June 4, 2024 22:29
WtfJoke added a commit to WtfJoke/setup-groovy that referenced this pull request Aug 1, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[eslint-plugin-github](https://github.com/github/eslint-plugin-github)
| [`4.10.2` ->
`5.0.1`](https://renovatebot.com/diffs/npm/eslint-plugin-github/4.10.2/5.0.1)
|
[![age](https://developer.mend.io/api/mc/badges/age/npm/eslint-plugin-github/5.0.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/eslint-plugin-github/5.0.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/eslint-plugin-github/4.10.2/5.0.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/eslint-plugin-github/4.10.2/5.0.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>github/eslint-plugin-github (eslint-plugin-github)</summary>

###
[`v5.0.1`](https://github.com/github/eslint-plugin-github/releases/tag/v5.0.1)

[Compare
Source](https://github.com/github/eslint-plugin-github/compare/v5.0.0...v5.0.1)

#### What's Changed

- chore(deps): bump the all-dependencies group with 5 updates by
[@&#8203;dependabot](https://github.com/dependabot) in
[github/eslint-plugin-github#530
- Provide `no-redundant-roles` exception for `rowgroup` by
[@&#8203;khiga8](https://github.com/khiga8) in
[github/eslint-plugin-github#531

**Full Changelog**:
github/eslint-plugin-github@v5.0.0...v5.0.1

###
[`v5.0.0`](https://github.com/github/eslint-plugin-github/releases/tag/v5.0.0)

[Compare
Source](https://github.com/github/eslint-plugin-github/compare/v4.10.2...v5.0.0)

Formally releasing v5.0.0!

This release includes everything in pre-release
[v5.0.0-2](https://github.com/github/eslint-plugin-github/releases/tag/v5.0.0-2)!

We notably dropped support for node 14 and node 16 in favor of node 18.

#### What's Changed

- Drop node 14/16 support and fix bug in `getElementType` logic by
[@&#8203;khiga8](https://github.com/khiga8) in
[github/eslint-plugin-github#525
- Bump node 14 to node 18 by
[@&#8203;khiga8](https://github.com/khiga8) in
[github/eslint-plugin-github#529
- chore(deps): bump the all-dependencies group with 4 updates by
[@&#8203;dependabot](https://github.com/dependabot) in
[github/eslint-plugin-github#510
- chore(deps): bump the all-dependencies group with 2 updates by
[@&#8203;dependabot](https://github.com/dependabot) in
[github/eslint-plugin-github#511
- chore(deps): bump the all-dependencies group with 2 updates by
[@&#8203;dependabot](https://github.com/dependabot) in
[github/eslint-plugin-github#512
- chore(deps): bump the all-dependencies group with 3 updates by
[@&#8203;dependabot](https://github.com/dependabot) in
[github/eslint-plugin-github#514
- chore(deps): bump the all-dependencies group across 1 directory with 4
updates by [@&#8203;dependabot](https://github.com/dependabot) in
[github/eslint-plugin-github#522

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "before 4am on Monday" in timezone
Europe/Berlin, Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job
log](https://developer.mend.io/github/WtfJoke/setup-groovy).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40MjEuMCIsInVwZGF0ZWRJblZlciI6IjM3LjQzOC4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->
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.

None yet

3 participants