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(css): improve color-mixer tool #193

Merged
merged 3 commits into from
Aug 23, 2024
Merged

Conversation

OnkarRuikar
Copy link
Contributor

The PR

  • makes color pickers keyboard navigable
  • fixed width for labels so UI layout won't change

@OnkarRuikar OnkarRuikar requested a review from a team as a code owner July 26, 2024 08:28
@OnkarRuikar OnkarRuikar requested review from pepelsbey and removed request for a team July 26, 2024 08:28
Copy link
Member

@pepelsbey pepelsbey left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you for improving the accessibility of this tool :)

I’d also suggest removing the role and tabindex attributes from buttons since they already have this role and are focusable by default. There’s one selector in the script that relies on the tabindex attribute, but it likely from the picker. I haven’t noticed any difference in the behavior.

Also, I noticed an error in DevTools because the source map file mentioned in the JS file is missing:

//# sourceMappingURL=pickr.es5.min.js.map

How about we remove this line to calm it down a bit? :)

tools/color-mixer/index.html Outdated Show resolved Hide resolved
tools/color-mixer/index.html Outdated Show resolved Hide resolved
@OnkarRuikar
Copy link
Contributor Author

There’s one selector in the script that relies on the tabindex attribute, but it likely from the picker.

Yes, it's from picker. And the current changes don't affect it.

@OnkarRuikar OnkarRuikar requested a review from pepelsbey August 23, 2024 03:02
Copy link
Member

@pepelsbey pepelsbey left a comment

Choose a reason for hiding this comment

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

Perfect! Thank you :)

@pepelsbey pepelsbey merged commit cd8a45a into mdn:main Aug 23, 2024
1 check passed
@OnkarRuikar OnkarRuikar deleted the css_color_mix branch August 24, 2024 04:00
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.

2 participants