-
Notifications
You must be signed in to change notification settings - Fork 117
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
Add lint rule against .tooltipped and introduce accessibility.yml config #2086
Conversation
🦋 Changeset detectedLatest commit: 073b0cf The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me 👍 Have you tested that it works without choking in dotcom?
Also, could you add a changeset?
I'll give it a try! And also fix the tests, which are breaking for some reason :( |
a59aa3c
to
d92a180
Compare
@camertron I started a test branch using the vendor script and pointing to this commit in dotcom. I updated the The one unexpected thing was the path which I had to set to get this config to work: Related commit. I did not expect to need to prefix the path to the config with |
Since this was last reviewed, I made these updates @primer/rails-reviewers:
Your input is appreciated here:
Thank you so much 🙏. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good including the ::
prefix 👍🏻
What are you trying to accomplish?
I started this rule in dotcom, but then I realized that PVC might make more sense so this rule can be shared across many projects.
This PR:
.tooltipped
in favor of alternatives, or the more recent tooltip.accessibility.yml
which will allow consumers to easily pull in recommended accessibility linting configurations which we can count against the scorecard. This will specifically contain rules that count towards the accessibility scorecard.base_linter.rb
to a helpers module.BaseLinter
seems to come with a lot of additional things this rule doesn't need, but I need to use these helpers.I think consumers can now add the following in their
.erb-lint.yml
to automatically pull in rules:Integration
No
List the issues that this change affects.
Closes https://github.com/github/accessibility/issues/3704
Risk Assessment
What approach did you choose and why?
accessibility.yml
config so consumers can easily configure recommended rules.Primer::Accessibility
.Anything you want to highlight for special attention from reviewers?
I namespaced the rule
Primer::Accessibility::
to align with what we do in erblint-github.The pro of this is that it can help to identify where a rule originates from. The con, is that the disable comment is kind of long (e.g. `erblint:disable Primer::Accessibility::TooltipHasMigration).
This also isn't consistent with the other rules living in PVC. What do you think? @primer/rails-reviewers?? 📣
I would also like to add docs for this rule but I'm not sure where it belongs, also given the doc migration work. What do you think? 📣
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.