-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Conversation
frontend/tailwind.config.js
Outdated
sub: "#262626", | ||
danger: "#E76A5E", | ||
success: "#A5E75E", | ||
subtle: "#454545", | ||
"subtle-alt": "#B7BDC2", |
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.
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
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 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
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.
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?)
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 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?
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.
Yeah sounds complicated :) I trust your judgement here, I don't need to be a blocker
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.
Just one comment
Changes
*-neutral-900
and*-[#171717]
class names withbg-base
(formerlybg-root-primary
)*-neutral-800
and*-[#262626]
class names withbg-base-secondary
(formerlybg-root-secondary
)primary
,tertiary
, andtertiary-light
colorssuccess
anddanger
colorsLink of any specific issues this addresses
To run this PR locally, use the following command: