-
Notifications
You must be signed in to change notification settings - Fork 13
feat: add new active color tokens to related components #1336
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @timowolf, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the styling capabilities for interactive elements, specifically buttons, by introducing distinct color tokens and CSS variables for their 'active' state. This change allows for more granular control over the visual feedback provided when a user engages with a button, improving the overall user experience. Concurrently, it updates a core branding dependency and includes a configuration adjustment to streamline package management. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces new active state color tokens and applies them to various button components. The changes involve updating the @simpl/brand dependency, adding new semantic tokens for active states, and updating the button styles to use these new tokens by separating :active styles from :hover styles.
The implementation is mostly correct, but I've found a critical issue where incorrect color tokens are used for the active state of secondary buttons, which would lead to incorrect styling. I've also pointed out a few medium-severity issues related to missing active text color definitions for some button types and inconsistent variable grouping, which could affect maintainability and cause potential accessibility problems. Please review the detailed comments for suggestions on how to address these points.
0ad2d77 to
8785c3d
Compare
525fcec to
8f1d4a3
Compare
|
Documentation. Coverage Reports: |
8f1d4a3 to
b723e33
Compare
panch1739
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dauriamarco Looks perfect!
The active state for the primary button in light mode, is not so visible. But this is just as designed and aligned with iX. If we get some feedback complaining about this, we can go back and revisit it with the rest of the team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @panch1739 👋 just a heads-up that after introducing the new colors for the active state, some a11y contrast issues are being reported by the Playwright tests.
The tests are failing with the following error:
color-contrast (serious): Ensure the contrast between foreground and background colors meets WCAG 2 AA minimum contrast ratio thresholds
At the moment, the failures seem to affect these components/pages:
I wanted to flag it so we can decide how to proceed/fix this. Let me know how you’d like to handle it 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dauriamarco mmm im confused, the components that is complaining about dont have an active state...is it because we are introducing here ALL the new colors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, for the badges and inline, i understand it complains about the danger version...but this is unrelated to the active states.
With the new colors, we should also resolve this as we are changing the values for text-danger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You’re right, sorry for the confusion 🙏 I mentioned the active state mostly because that’s what I was working on, and chronologically I noticed these failures right after introducing it, but it’s not directly related to the active state itself.
Some existing colors have slightly changed, and that caused visual diffs for many components (e.g. filtered-search) and, in some cases, contrast violations (e.g. badges or anything using text-danger).
So I guess we should already resolve this here as I need the new colors, including the new danger color, for the active states. Or am I missing something?
b723e33 to
3ac9c91
Compare
Updates a set of UI components to introduce a new active state based on the latest design tokens.
The active state has been added or updated for the following components: