-
Notifications
You must be signed in to change notification settings - Fork 97
fix(theming)!: remove background.primary color variable
#1938
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
background.primary color variablebackground.primary color variable
| surfaceColor={ | ||
| highlightedValue === item.value ? 'background.primary' : 'background.raised' | ||
| highlightedValue === item.value | ||
| ? 'background.primaryEmphasis' |
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'm waiting for the PR to deploy, but I don't think this is what we want to demonstrate with this pattern. The idea is to get the avatar's surface to match the active menu item as close as possible. So, it should be something like getColor({ theme, hue: 'primaryHue', light: { shade: 100}, dark: { shade: 900} }) to correspond with the previous variable values.
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.
Or maybe getColor({ theme, variable: 'background.primaryEmphasis', transparency: 100 }) is a more successful match 🤷
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.
Or maybe getColor({ theme, variable: 'background.primaryEmphasis', transparency: 100 }) is a more successful match 🤷
getColor will throw because the output of the ☝️ is rgba() and it's treated as a color variable ($surfaceColor.includes('.')). We would have to refine the logic everywhere this comes up.
ReferenceError: Error: color variable 'rgba(31,115,183,0.08)' is not defined
Also, with the way Avatar is built, the transparent surfaceColor would lay on top of the background color. That wouldn't result in the right color.
packages/avatars/src/types/index.ts
Outdated
| * Provides surface color for an avatar placed on a non-default background. | ||
| * Accepts a [color variable](/components/theme-object#colors) key (i.e. | ||
| * `background.primary`) to render based on light/dark mode, or any hex value. | ||
| * `background.primaryEmphasis`) to render based on light/dark mode, or any hex value. |
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.
| * `background.primaryEmphasis`) to render based on light/dark mode, or any hex value. | |
| * `background.emphasis`) to render based on light/dark mode, or any hex value. |
[nit] because we don't want to suggest that primaryEmphasis should be used for anything other than primary interactive elements.
Description
Following @lucijanblagonic's Figma comment,
background.primaryisn't relevant to Garden consumers and should be removed.Details
background.primaryfromThemebackground.primarywithbackground.primaryEmphasisChecklist
npm start)⬅️ renders as expected with reversed (RTL) direction?bedrock)~♿ tested for WCAG 2.1 AA accessibility compliance