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/contrast checker #847

Merged
merged 43 commits into from
Feb 19, 2023
Merged

Conversation

ryceg
Copy link
Contributor

@ryceg ryceg commented Jan 18, 2023

Before submitting the PR:

  • Does your PR reference an issue? If not, please chat to the team on Discord or GitHub before submission.
  • Did you update and run tests before submission using npm run test?
  • Does your branch follow our naming convention? If not, please amend the branch name using branch -m new-branch-name
  • Did you update documentation related to your new feature or changes?

What does your PR address?

Adds a contrast checker that allows the user to quickly make sure that their theme meets WCAG AA or AAA guidelines. Tooltip allows them to hover for more info (but is currently not updating when you change color)

@vercel
Copy link

vercel bot commented Jan 18, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
skeleton-docs ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 19, 2023 at 7:24PM (UTC)

@endigo9740
Copy link
Contributor

endigo9740 commented Jan 18, 2023

Quick feedback:

  1. Using the current theme colors for success/warning/error falls apart for randomization. I'd recommend using hardcoded green/yellow/red instead to indicate status. They may not not play well with the theme, but that is ok, they're not part of the theme.
  2. Not sure I understand the text crammed into the label text. It's breaking the layout. I'd suggest keeping this info in your tooltip
    • image
  3. I should have been more specific - I meant native tooltip attributes not our utilities. This is a temporary home as I feel the tooltips may undergo some structural changes and should not be relied on atm. If you absolutely must keep them ensure they use a neutral color - even bg-neutral-900 would be fine here.
  4. There's little reason to use colored emoji when we have a proper icon library inserted now. We can use any on Font Awesome that fall under "free". And with the proper on-color these should remain readable. Right now we're getting the "clown" effect where everything is so colorful it's grabbing all the attention. This is a tool, not the main attraction.
  5. The use of buttons is odd. I might suggest badges, which can be much smaller by default.
  6. Nik brought this up, but I also didn't understand the check mark. Again positive/negative should be the focus, with more info for power users in the tooltip. Anything more than that and we'll have to introduce a legend for normal users.

Ping me when these changes are in and I'll review again. Thanks!

@endigo9740
Copy link
Contributor

endigo9740 commented Jan 19, 2023

@ryceg I know this is still a work in progress, but a few small issues:

  • Position is off. This needs to be bumped to the left a bit to overlap. Test with/without rounded inputs too.
  • The badge is oblong (taller than it is wide). Not sure why.
  • When hovering there needs to be a space after the RGB values in the title

Screen Shot 2023-01-19 at 1 39 22 PM

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.

None yet

2 participants