-
Notifications
You must be signed in to change notification settings - Fork 97
feat(loaders): new light/dark mode colors #1818
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
Conversation
| <div | ||
| style={{ | ||
| backgroundColor: backgroundColor || (args.isLight ? PALETTE.kale[600] : undefined), | ||
| backgroundColor: backgroundColor || (args.isLight ? PALETTE.kale[800] : undefined), |
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.
Bumping the shade in accordance to the handy-dandy v8-to-v9 color mapping tool. 👍
@ze-flo Could you expand a bit on why that is the case?
The color on this one seems a bit off, it should be
Overall, the And another question, is there an easy way to see which variable is being used and where in Storybook? Inspector is showing the color value only, and Variables section shows main variables only. |
Based on @jzempel's comment, we need to align on whether we should keep the color inheritance or set the default color ourselves.
Absolutely doable! 💯 Let me recap for clarity: Regular SkeletonLight mode
Dark mode
Light SkeletonLight mode
Dark mode
@lucijanblagonic Is this correct?
Sadly, it's impossible at this time. 😞 Garden components do not reference CSS variables. Instead they get their calculated color values at runtime from our |
Light mode should also be
OK, so we can only review color values, and we are counting on eng to implement correct variables (e.g. progress loaders might use |
| const backgroundColor = getColor({ | ||
| theme, | ||
| hue: 'neutralHue', | ||
| transparency: theme.opacity[200], | ||
| light: { shade: 700 }, | ||
| dark: { shade: 500 } | ||
| }); | ||
| const foregroundColor = color || getColor({ theme, variable: 'border.successEmphasis' }); |
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, so we can only review color values, and we are counting on eng to implement correct variables (e.g. progress loaders might use bg group, not border and it should be 100% clear from the inventory)?
@lucijanblagonic I always define colors using the variables found in the inventory first.
For Progress bar, the foregroundColor uses border.successEmphasis. It exists in our collection of border variables so I used it.
The backgroundColor uses WIP_opacity/neutral (700 : 500)/200, which is the same as foreground.subtle. However, because we don't have a background variable that matches the hue / shade combo per mode, I build it using advanced getColor options.
I hope this answers your question. If this isn't the right approach, let me know.
Thanks!
| }: IStyledProgressBackgroundProps & ThemeProps<DefaultTheme>) => { | ||
| const backgroundColor = getColor({ | ||
| theme, | ||
| hue: 'neutralHue', |
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.
Should this inherit from a background variable? 🤔
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.
As discussed with Design, we're not tying this one to a variable, but rather build it from scratch.
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.
fwiw, I think @geotrev's point of tying this to foreground.subtle actually does make sense as the background of this loader actually functions as a subtle foreground color when Progress is applied to a page. Let's not make any adjustments now, but I want to reserve space to triple-check semantics with design before v9 wraps up.
|
|
||
| const PULSE_ANIMATION = keyframes` | ||
| 0%, 100% { | ||
| opacity: .2; | ||
| const retrieveAnimation = ({ theme }: ThemeProps<DefaultTheme>) => keyframes` | ||
| 0% { | ||
| opacity: 1; | ||
| } | ||
| 50% { | ||
| opacity: .8; | ||
| opacity: ${theme.opacity[600]}; | ||
| } | ||
| 100% { | ||
| opacity: ${theme.opacity[200]}; | ||
| } |
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.
Why did the 0% portion of the animation change?
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.
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, I checked with @lucijanblagonic and he wasn't expecting the animation to materially change based on Figma. And now we're seeing some odd animation with the Inline loader (no longer smooth as it was before). I think we should revert here (while retaining the theme.opacity usage).
| }: IStyledProgressBackgroundProps & ThemeProps<DefaultTheme>) => { | ||
| const backgroundColor = getColor({ | ||
| theme, | ||
| hue: 'neutralHue', |
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.
fwiw, I think @geotrev's point of tying this to foreground.subtle actually does make sense as the background of this loader actually functions as a subtle foreground color when Progress is applied to a page. Let's not make any adjustments now, but I want to reserve space to triple-check semantics with design before v9 wraps up.





Description
Adds dark / light mode support for
loaders.Detail
loadersImportant
Spinner,Dots, andInlinenow default to the agreed mode-specific color rather than inheriting it from the parent container.Checklist
npm start)⬅️ renders as expected with reversed (RTL) direction🤘 renders as expected with Bedrock CSS (?bedrock)♿ tested for WCAG 2.1 AA accessibility compliance