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

Added allowedInvalidRoles option to aria-role #828

Merged

Conversation

JoshuaKGoldberg
Copy link
Contributor

Adds an optional allowedInvalidRoles: string[] option for an allowlist of roles to no longer consider invalid.

Fixes #574.

@codecov
Copy link

codecov bot commented Dec 2, 2021

Codecov Report

Merging #828 (1df7f13) into master (30deacb) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 1df7f13 differs from pull request most recent head 566011b. Consider uploading reports for the commit 566011b to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #828   +/-   ##
=======================================
  Coverage   99.22%   99.22%           
=======================================
  Files          98       98           
  Lines        1417     1419    +2     
  Branches      477      479    +2     
=======================================
+ Hits         1406     1408    +2     
  Misses         11       11           
Impacted Files Coverage Δ
src/rules/aria-role.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

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

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM with minor comments

docs/rules/aria-role.md Outdated Show resolved Hide resolved
src/rules/aria-role.js Outdated Show resolved Hide resolved
@JoshuaKGoldberg
Copy link
Contributor Author

Ping @ljharb - could I request a re-review please?

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM!

@ljharb ljharb force-pushed the aria-role-allowed-invalid-roles branch from 1df7f13 to 566011b Compare January 6, 2022 18:11
@ljharb ljharb merged commit 566011b into jsx-eslint:master Jan 6, 2022
@JoshuaKGoldberg JoshuaKGoldberg deleted the aria-role-allowed-invalid-roles branch January 6, 2022 18:21
@coreyward
Copy link

Any idea when this will be published?

@ljharb
Copy link
Member

ljharb commented Jan 31, 2022

Nope, no idea. It will be included in the next release, whenever that is.

@johan
Copy link

johan commented Feb 21, 2022

It would be lovely to get a v6.6.0 published from main with this, so we can use npm version number dependencies rather than git urls to use the feature.

(+ A bit confusing to find the option in the official docs but not be able to use it in the latest published version, three weeks after it landed.)

@ljharb
Copy link
Member

ljharb commented Feb 21, 2022

Re the last part, for any GitHub repo, docs on the default branch often contain unpublished changes - you need to check under the tag.

@coreyward
Copy link

I encourage maintainers to circle back to issues and PRs with a comment mentioning the version that includes a fix or merge. Since a lot of users will find issue pages while trying to self serve, the additional information helps reduce confusion and create awareness without too much extra work.

@ljharb
Copy link
Member

ljharb commented Feb 24, 2022

That’s unsustainable to do manually, and my experience with bots that auto comment on release is that they’re excessively noisy.

Unfortunately the best outcome would come with user education about how to understand when a closed issue/PR is, or isn’t, released.

@coreyward
Copy link

In practice, it's not bad. I also release when there are changes that fix issues. I find that proactive communication cuts down on the volume of pings, asks, and follow-up issues. A little extra work to solve a lot of extra work makes sense to me, but obviously you are welcome to do things however works best for you, I'm just sharing what I've learned.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

role="text" should be valid
5 participants