-
Notifications
You must be signed in to change notification settings - Fork 26
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
fix: migrate eslintrc to flag config #136
Conversation
Many thanks for your contribution. |
I added a step to lint all files as most of the issues pretain to the rules requiring 'app' be prefixed to a component's selector. I have gone ahead and removed these rules as none of the components have this prefix as part of 6e6ee39. These linting rules are probably do for a refinement at somepoint as there is a huge hodgepodge. |
All linting rules have been resolved and it can not be enforced in the CI pipeline. It would be good if someone who has some knowledge of accessibility can review 6fde327. If there are no other comments or concerns, could we get this merged @spike-rabbit ? |
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.
Thx a lot for your contribution. We already had this on our agenda, so your PR is highly appreciated.
Please fix my review comment and do a rebase on our latest master. Then this should be ready to go.
I think your a11y changes are fine. This anyway requires a way more fixes.
Just one general note, we have our shared ruleset, which I tried enable based on your changes. But it was causing a 100+ findings. So I think just going with the rules defined here makes sense and moving the integration of our ruleset to another PR.
d2aa88a
to
950f5c7
Compare
@spike-rabbit I have reverted the changes to the CI pipeline and rebased onto master. I agree that switching to shared config should be handled in a separate PR as the purpose of this was just to fix eslint. |
Awesome, FYI I only added the updated screenshots. Removing auto focus actually solved a bug related to scrolling. Thx again a lot for your contribution 🚀 |
What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
In regards to #135.
What is the new behavior?
Migrate to the new config format so all files can be linted.
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: