-
Notifications
You must be signed in to change notification settings - Fork 0
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: update color tokens for modal component #291
Conversation
✅ Deploy Preview for shidoka-applications ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -169,4 +172,5 @@ h1 { | |||
overflow-y: auto; | |||
margin: -4px; | |||
padding: 4px 16px 4px 4px; | |||
background: var(--kd-color-background-accent-subtle); |
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.
I don't think we actually want a background color on the body here. I think that was just done in Figma to indicate a slot.
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.
Sure @brian-patrick-3 . Will check with Chandan
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.
Confirmed with UX. Removed the background color on the body
I think the designers also added some styles for the X button states? |
@brian-patrick-3 , included these styles. |
Hmm, they don't seem to match Figma at all to me |
Yes, they look different , may be because it blends with the modal background color . But I used same tokens as in figma : |
I think the colors are correct, but the size and shape of the button, and the outline/border thickness on active/focus, seems wrong. |
border-radius: 8px; | ||
border: 1px solid var(--kd-color-border-light); | ||
border: 1px solid var(--kd-color-border-variants-light); | ||
box-shadow: 0px 2px 12px 0px rgba(61, 60, 60, 0.25); |
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.
This box-shadow shouldn't be needed, as Elevation is already included.
Pushed the changes |
} | ||
|
||
&:active { | ||
color: var(--kd-color-text-pressed); | ||
border-radius: 2px; | ||
border: 1px solid var(--kd-color-border-ui-pressed); |
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.
This is causing the button size to change when it is clicked.
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.
Size remains same but it appears to shift slightly . Because focus state has only outline, while pressed state has a border + outline
Please let me know if it should be either border for both states or outline.
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.
It can have both. To fix the size shift, you either need to set fixed width/height on the button (box-sizing: border-box
from global.scss makes this work), or give it a 1px transparent border in the default state.
In figma, I only see a 1px border for both pressed and focus.
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.
Added 1px transparent border . Thanks Brian
Hi @tulasigudibanda, |
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.
test comment
@srpriyankashetty , |
Verified |
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.
Verified Modal component in Light and dark mode theme and looks fine
Verified the OK button actions and color changes in Modal, Action button, before close and with form and looks fine
Verified the hide close and looks fine
Verified the Secondary button color in both dark and light mode theme
🎉 This PR is included in version 2.0.0-next.28 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Updated to theme based color tokens.
ADO Story or GitHub Issue Link
TASK 2003584
Figma Link
https://www.figma.com/design/CQuDZEeLiuGiALvCWjAKlu/branch/qMpff4GuFUEcsMUkvacS3U/Applications---Component-Library?node-id=11285-770&node-type=instance&m=dev
Notes
Updated tokens in below components :
Modal