-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
UI: Update colors for 7.0 #19023
UI: Update colors for 7.0 #19023
Conversation
…n many components
code/lib/theming/src/global.ts
Outdated
@@ -11,6 +12,7 @@ interface Return { | |||
export const createReset = memoize(1)( | |||
({ typography }: { typography: Typography }): Return => ({ | |||
body: { | |||
color: color.defaultText, |
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.
Is this the correct place to set the body color?
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.
Is it getting set elsewhere right now?
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.
@domyen It seems to not be set anywhere, though line 104 of that file looks like it MIGHT be a place that is supposed to set it, but isn't.
code/lib/theming/src/global.ts
Outdated
@@ -11,6 +12,7 @@ interface Return { | |||
export const createReset = memoize(1)( | |||
({ typography }: { typography: Typography }): Return => ({ | |||
body: { | |||
color: color.defaultText, |
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.
Is it getting set elsewhere right now?
@domyen This was intentional to match the mockups and proposal. I will try increasing the contrast a touch.
We decided to go this route when designing the Interactions addon. I can change this if you prefer a different background color. |
@@ -6,11 +6,11 @@ const theme: ThemeVars = { | |||
|
|||
// Storybook-specific color palette | |||
colorPrimary: '#FF4785', // coral | |||
colorSecondary: '#1EA7FD', // ocean | |||
colorSecondary: '#029CFD', // ocean |
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.
could be helpful to have another object which are aliases like
{
ocean: colors.colorSecondary
...
}
@@ -6,7 +6,7 @@ const theme: ThemeVars = { | |||
|
|||
// Storybook-specific color palette | |||
colorPrimary: '#FF4785', // coral | |||
colorSecondary: '#1EA7FD', // ocean | |||
colorSecondary: '#029CFD', // ocean |
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.
seems like these are shared between themes - could be exported as constants in base and used in the light and dark files
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.
Great! On it!
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 ran into a hiccup. I implemented it as you suggested, but convert.js
gets a bit mad unless I add them there. I am not sure the best way to do that.
@@ -71,8 +71,11 @@ const Label = styled.label(({ theme }) => ({ | |||
}, | |||
|
|||
'input:checked ~ span:last-of-type, input:not(:checked) ~ span:first-of-type': { | |||
background: theme.background.app, | |||
boxShadow: `${opacify(0.1, theme.appBorderColor)} 0 0 2px`, | |||
background: theme.boolean.selectedBackground, |
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.
seems like there are two conventions going on,
when I saw all the text prefixes like textMutedColor, textColor, etc I had an urge to comment that it could be text.muted etc.
I also see ones like positiveText, warningText etc. so maybe there are 3 naming conventions.
I know these came before this PR but just highlighting it in case this is an easy-ish time to standardize some
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.
forgot about this specific line - it could be booleanSelectedBg or booleanSelectedBackground
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 am hesitant to change the names of existing variables as I don't want to break user themes that rely on these names. I am open to suggestions on preferred naming schemes. If we do decide to rename them (breaking change), now is the time.
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 some consistency/ergonomics comments, overall looks good!
Alrighty, I think this one is good to go. Any further refinements I will do in followup PRs. |
@shilman I believe this one is ready to go. |
What I did
Updated the base colors, light color scheme, and dark color scheme.
Updates to base.ts:
color.secondary
, the monochrome colors,color.border
, andbackground.app
positiveText
,negativeText
, andwarningText
Update to convert.ts:
button
andboolean
objects with their own customizable background/border colors. They fall back to input background colors, but overall should make theming form elements simpler.Notes:
code/lib/components/src/Colors/colorpalette.stories.mdx
seems to be broken, but is unrelated to this PR.How to test
If your answer is yes to any of these, please make sure to include it in your PR.