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: use new color palette #536

Draft
wants to merge 7 commits into
base: release
Choose a base branch
from
Draft

Conversation

tanvibhakta
Copy link
Contributor

@tanvibhakta tanvibhakta commented Jul 26, 2023

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

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Manual on storybook
  • Manual on main repo
  • Jest
  • Cypress

Test Plan

Add Testsmith test cases links that relate to this PR

Issues raised during DP testing

Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR)

Checklist:

Dev activity

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

QA activity:

  • Test plan has been approved by relevant developers
  • Test plan has been peer reviewed by QA
  • Cypress test cases have been added and approved by either SDET or manual QA
  • Organized project review call with relevant stakeholders after Round 1/2 of QA
  • Added Test Plan Approved label after reveiwing all Cypress test

@changeset-bot
Copy link

changeset-bot bot commented Jul 26, 2023

⚠️ No Changeset found

Latest commit: 8c1f67c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Jul 26, 2023

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

Name Status Preview Comments Updated (UTC)
design-system ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 3, 2023 2:04pm

@tanvibhakta
Copy link
Contributor Author

@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 ™️.

@albinAppsmith
Copy link
Contributor

Screenshot 2023-07-28 at 3 14 38 PM

Button have a different border color

/** 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);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be 500?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@albinAppsmith
Copy link
Contributor

albinAppsmith commented Jul 28, 2023

Screenshot 2023-07-28 at 3 17 01 PM

Is this active state correct?
cc: @vasanthappsmith

@vasanthappsmith
Copy link
Contributor

vasanthappsmith commented Jul 28, 2023

https://theappsmith.slack.com/archives/C0293DVQACW/p1689573934172429?thread_ts=1689477001.434849&cid=C0293DVQACW

During our discussion, we came to the conclusion that the Red-Error button does not fit well with the application in Modal and other places, with secondary action buttons. It also has very poor contrast with the background and text. We tried altering the hues of the color like 50, 100, and 200 to fit, but found that this does not work well for other use-cases. Hence, we suggest having a darker button. Attaching options for the button variants.

Based on the conversation we had last time. You suggested we should have a separate task on this.
image

@tanvibhakta
Copy link
Contributor Author

@vasanthappsmith

You suggested we should have a separate task on this.

This does not mean that it is okay to break the current experience of the error button.

@vasanthappsmith
Copy link
Contributor

@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?

@tanvibhakta
Copy link
Contributor Author

What do you suggest then? A quick fix?

No, we need a holistic solution, this is why we allocated all the time we did towards color palette

Also, I don't remember having a border for buttons.

This is why we we don't rely on memory when making design changes, we rely on existing documentation, and prototyping.

If it's hard to replace now. Can we have a '-500' for the buttons?

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

@vasanthappsmith
Copy link
Contributor

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

@tanvibhakta tanvibhakta marked this pull request as draft August 1, 2023 04:19
@tanvibhakta
Copy link
Contributor Author

/generate-alpha

@github-actions
Copy link
Contributor

github-actions bot commented Aug 3, 2023

Publish Result: @appsmithorg/design-system@0.0.0-alpha-20230803140807

@dpulls
Copy link

dpulls bot commented Aug 12, 2023

🎉 All dependencies have been resolved !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Task]: Update Color Palette
3 participants