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

[feat] a11y-no-redundant-roles #7067

Merged
merged 17 commits into from
Jan 3, 2022

Conversation

jamesb93
Copy link

This solves one task on the list in #820. My first PR so probably a lot going on wrong here but I would be very appreciative for feedback and guidance on contributing.

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

src/compiler/compile/nodes/Element.ts Outdated Show resolved Hide resolved
src/compiler/compile/nodes/Element.ts Outdated Show resolved Hide resolved
src/compiler/compile/nodes/Element.ts Outdated Show resolved Hide resolved
src/compiler/compile/nodes/Element.ts Outdated Show resolved Hide resolved
src/compiler/compile/compiler_warnings.ts Outdated Show resolved Hide resolved
@jamesb93
Copy link
Author

jamesb93 commented Dec 30, 2021

Thanks for your feedback @bluwy. Youre suggestions and enhancements have been implemented in db606c7, 91ec3fc, 53b8919, 17906c2.

src/compiler/compile/nodes/Element.ts Outdated Show resolved Hide resolved
src/compiler/compile/nodes/Element.ts Outdated Show resolved Hide resolved
@dummdidumm
Copy link
Member

I just realized there exists a PR already for this: #5361 . Though I'm not sure which implementation should be preferred.

@bluwy
Copy link
Member

bluwy commented Dec 31, 2021

Crap now I feel bad for choosing either one since both has its good parts and effort put into it. #5361 does have more checks. On the other hand, this PR also handles parent section/article checks. If I have to choose, I think we should continue this, and borrow over the extra checks from #5361 to here. We can then have @mhatvan as the co-author in the commit description.

@jamesb93
Copy link
Author

Crap now I feel bad for choosing either one since both has its good parts and effort put into it. #5361 does have more checks. On the other hand, this PR also handles parent section/article checks. If I have to choose, I think we should continue this, and borrow over the extra checks from #5361 to here. We can then have @mhatvan as the co-author in the commit description.

I'd be more than happy to update with work already done in #5361. I think the joining of the two would make for a robust set of checks.

@jamesb93
Copy link
Author

I've updated this PR with the conditions that are checked in PR #5361 @bluwy + @dummdidumm. I've also updated the logic so that there isn't a massive if statement with numerous nested ||. Instead there is now a Map() that contains the many cases that can arise with anything else being caught by this.name === value.

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Thanks for further improving the PR! I've left some comments to help with performance, but after that this looks good to me.

src/compiler/compile/nodes/Element.ts Outdated Show resolved Hide resolved
src/compiler/compile/nodes/Element.ts Outdated Show resolved Hide resolved
Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

Thanks for keeping up with the many requested changes 😅 LGTM with one tiny nitpick.

src/compiler/compile/nodes/Element.ts Outdated Show resolved Hide resolved
@mcmxcdev
Copy link
Contributor

Well, I tried to get attention to my PR without success for a while. Anyway, I am fine with having this PR merged with stuff taken from mine ;)
The most important part is to get more a11y checks into svelte!

@jamesb93
Copy link
Author

Well, I tried to get attention to my PR without success for a while. Anyway, I am fine with having this PR merged with stuff taken from mine ;) The most important part is to get more a11y checks into svelte!

It is definitely important to acknowledge that a significant portion of the code was championed by you @mhatvan. I learned from it and it looks like the checks should be pretty robust as a result. Thank you :)

@mcmxcdev
Copy link
Contributor

Thanks man, I appreciate it!

All that matters is more and improved a11y checks ;)

@jamesb93
Copy link
Author

jamesb93 commented Jan 3, 2022

It seems like one specific test (unrelated perhaps to this PR) is failing on macOS-latest and node 12. Is there any guidance on making it so this PR is fully test compliant?

@dummdidumm
Copy link
Member

Don't worry about that, according to CI they just timed out.

@dummdidumm dummdidumm merged commit 84a4ef0 into sveltejs:master Jan 3, 2022
@Conduitry
Copy link
Member

Released in 3.45.0 - thank you!

@jamesb93
Copy link
Author

jamesb93 commented Jan 6, 2022

Released in 3.45.0 - thank you!

Woo! Thanks everyone for your guidance and hard work. Much appreciated and I learned a lot.

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.

5 participants