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

chore(frontend): Standardize custom colors used throughout the app #6833

Merged
merged 3 commits into from
Feb 20, 2025

Conversation

amanape
Copy link
Member

@amanape amanape commented Feb 19, 2025

Changes

  • Replace all instances of *-neutral-900 and *-[#171717] class names with bg-base (formerly bg-root-primary)
  • Replace all instances of *-neutral-800 and *-[#262626] class names with bg-base-secondary (formerly bg-root-secondary)
  • Define new primary, tertiary, and tertiary-light colors
  • Change success and danger colors

Link of any specific issues this addresses


To run this PR locally, use the following command:

docker run -it --rm   -p 3000:3000   -v /var/run/docker.sock:/var/run/docker.sock   --add-host host.docker.internal:host-gateway   -e SANDBOX_RUNTIME_CONTAINER_IMAGE=docker.all-hands.dev/all-hands-ai/runtime:7e8bfdf-nikolaik   --name openhands-app-7e8bfdf   docker.all-hands.dev/all-hands-ai/openhands:7e8bfdf

@amanape amanape self-assigned this Feb 19, 2025
@amanape amanape changed the title Standardize Tailwind CSS colors chore(frontend): Standardize TailwindCSS colors Feb 19, 2025
@amanape amanape changed the title chore(frontend): Standardize TailwindCSS colors chore(frontend): Standardize custom colors used throughout the app Feb 19, 2025
Comment on lines 14 to 18
sub: "#262626",
danger: "#E76A5E",
success: "#A5E75E",
subtle: "#454545",
"subtle-alt": "#B7BDC2",
Copy link
Collaborator

@rbren rbren Feb 20, 2025

Choose a reason for hiding this comment

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

are there better words for sub, subtle and subtle-alt? Even color names like light-gray and medium-gray might be easier to reason about

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to avoid titles that include the color such as light-gray and medium-gray since they may be changed and not to be confused with Tailwind default color class names (text-gray-500, text-white, etc)

root -> base
sub -> base-secondary
subtle -> tertiary
subtle-alt -> tertiary-light

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK. Not a hill I'm going to die on :) But I'm not sure a new person would know when to use which, if there's any more guidance we can provide (e.g. are these always background colors? text colors?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I could add comments in the config definitions. The colors themselves aren't really limited to certain use cases. For example, the tertiary color is used for input borders and placeholder text. base-secondary is mostly for the background color, but also for tooltips.

We could revert base -> background-primary and base-secondary to background-secondary; but not sure what we could name the tertiary and tertiary-light. If we use terms like "border" or "text" in the color, it will look unpleasant (e.g., the class name would be text-[key], which could end up looking text-text or border-border

WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah sounds complicated :) I trust your judgement here, I don't need to be a blocker

Copy link
Collaborator

@rbren rbren left a comment

Choose a reason for hiding this comment

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

Just one comment

@amanape amanape enabled auto-merge (squash) February 20, 2025 15:34
@amanape amanape disabled auto-merge February 20, 2025 15:42
@amanape amanape enabled auto-merge (squash) February 20, 2025 15:52
@amanape amanape merged commit 2f14e53 into main Feb 20, 2025
15 checks passed
@amanape amanape deleted the ALL-1325/standardize-colors branch February 20, 2025 16:13
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