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

fix: migrate eslintrc to flag config #136

Merged
merged 3 commits into from
Feb 3, 2025

Conversation

grantwforsythe
Copy link

@grantwforsythe grantwforsythe commented Jan 1, 2025

What kind of change does this PR introduce? (check one with "x")

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe: Linting

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")

  • Yes
  • No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

@timowolf
Copy link
Member

timowolf commented Jan 2, 2025

Many thanks for your contribution.

@CLAassistant
Copy link

CLAassistant commented Jan 21, 2025

CLA assistant check
All committers have signed the CLA.

@fh1ch fh1ch requested a review from spike-rabbit January 29, 2025 18:22
@grantwforsythe
Copy link
Author

grantwforsythe commented Jan 30, 2025

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.

image

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.

@grantwforsythe
Copy link
Author

grantwforsythe commented Jan 30, 2025

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 ?

Copy link
Member

@spike-rabbit spike-rabbit left a 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.

@grantwforsythe grantwforsythe force-pushed the master branch 3 times, most recently from d2aa88a to 950f5c7 Compare February 2, 2025 03:31
@grantwforsythe
Copy link
Author

grantwforsythe commented Feb 2, 2025

@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.

@spike-rabbit
Copy link
Member

Awesome, FYI I only added the updated screenshots. Removing auto focus actually solved a bug related to scrolling.
I also squashed some of your commits and added the messages. I did not change any code / config.

Thx again a lot for your contribution 🚀

@spike-rabbit spike-rabbit merged commit de4664e into siemens:master Feb 3, 2025
3 checks passed
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.

4 participants