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

test(color-picker): add component token E2E tests #9737

Conversation

jcfranco
Copy link
Member

@jcfranco jcfranco commented Jul 5, 2024

Related Issue: #7180

Summary

✨🧪✨

@jcfranco jcfranco requested a review from a team as a code owner July 5, 2024 16:07
Copy link
Contributor

@alisonailea alisonailea left a comment

Choose a reason for hiding this comment

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

I opened the tests in debugger mode and it looks like several of the tokens are not applied?

@jcfranco
Copy link
Member Author

jcfranco commented Jul 16, 2024

@alisonailea I've removed some of the unused props you pointed out. Thanks for the catch! ✨🎣✨

Others that were pointed out were working, but not shown up for specific configurations or because there was an issue with styles. I've also fixed these and here's how they're looking:

Screenshot 2024-07-16 at 3 09 15 PM

A couple of questions on ☝️:

  1. the background/text color of prefix and suffixes should be the same, so WDYT about consolidating these to --calcite-color-picker-affix-background-color and --calcite-color-picker-affix-text-color?
  2. both button icon/text color props can be applied based specificity and currentColor: should we drop the icon prop in favor of the button-text prop or, following your suggestion, rely on the component's text color prop? The buttons do change background color when hovered, so it might be useful to allow customizing this differently from the component's text color.

@alisonailea
Copy link
Contributor

  1. yes I think --calcite-color-picker-affix-text-color makes sense
  2. I'm all for dropping extraneous tokens

@jcfranco jcfranco requested a review from alisonailea July 16, 2024 22:58
@jcfranco
Copy link
Member Author

@alisonailea Updated. Would you mind reviewing this again? 🙏

@jcfranco jcfranco merged commit ad9b49a into epic/7180-component-tokens Jul 17, 2024
4 checks passed
@jcfranco jcfranco deleted the jcfranco/7180-add-color-picker-component-token-e2e-tests branch July 17, 2024 15:26
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