-
Notifications
You must be signed in to change notification settings - Fork 28
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: use new color palette #536
base: release
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@vasanthappsmith needs to hand off all the updates to our existing semantic tokens before this PR can go in. This PR is blocking #538. When this PR goes in, the tag PR will Just Work ™️. |
/** Used as active border hover color of secondary items like radio checkbox etc */ | ||
--ads-v2-color-border-brand-secondary-emphasis: var(--ads-v2-color-gray-700); | ||
/** Used as active border color of secondary items like radio checkbox etc */ | ||
--ads-v2-color-border-brand-secondary: var(--ads-v2-color-gray-600); | ||
/** Used as default border color when --ads-v2-color-bg-brand is the default bg */ | ||
--ads-v2-color-border-brand: var(--ads-v2-color-orange-600); | ||
--ads-v2-color-border-brand: var(--ads-v2-color-orange-300); |
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 should be 500?
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 looks like this is the value in the semantic tokens list.
https://www.notion.so/appsmith/Semantic-Color-Token-174c8017d8e5481b833805c22345f493?pvs=4#b83feb4239a0479c86c6c25b82710f45
cc @vasanthappsmith is there a change that needs to be made here?
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.
@tanvibhakta: Why do we need a border for the primary button? Is there a specific reason for this?
@albinAppsmith: We currently use -300 for borders, which is mostly for tags. We should not apply it to buttons.
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.
@vasanthappsmith, we need a border for the primary button because these are the designs you gave us when we were building the components during the ADS 2.0 project phase one.
We will need to keep in mind that semantic tags are used everywhere, including many places in the main repo we don't have control over.
Is this active state correct? |
This does not mean that it is okay to break the current experience of the error button. |
@tanvibhakta What do you suggest then? A quick fix? Also, I don't remember having a border for buttons. If it's hard to replace now. Can we have a '-500' for the buttons? |
No, we need a holistic solution, this is why we allocated all the time we did towards color palette
This is why we we don't rely on memory when making design changes, we rely on existing documentation, and prototyping.
No, we use semantic tokens in all the places, and we cannot replace it with a primary token, that would defeat the purpose of us having a design system and a token hierarchy. I think your next steps here should be to go through every state and every component in the preview here, make a note of the changes that are needed, and then you can go back to the drawing board. cc @AbhaySM |
Sure, I can do that. Do we have different semantics for "fg" icons? We should have them since we need to differentiate between the icons and text for callouts/toasts. For emphasized text, we will also use 700, which is for the tags and callout text. cc: @AbhaySM |
chore: Release chromatic fetch depth flow
chore: Release workflow
/generate-alpha |
Publish Result: @appsmithorg/design-system@0.0.0-alpha-20230803140807 |
🎉 All dependencies have been resolved ! |
Description
handoff for hex values: https://www.notion.so/appsmith/New-Colors-86633b9e99674c2db9c63587c1fc7376
handoff for semantic token changes: https://www.notion.so/appsmith/Semantic-Color-Token-174c8017d8e5481b833805c22345f493
Fixes appsmithorg/appsmith#25719
Depends on appsmithorg/appsmith#25747
Type of change
How Has This Been Tested?
Test Plan
Issues raised during DP testing
Checklist:
Dev activity
QA activity: