fix(theming)!: remove background.primary color variable
#1938
Conversation
ze-flo
changed the title
background.primary color variablebackground.primary color variable
| @@ -64,7 +64,9 @@ export const MenuStory: StoryFn = ({ isCompact }) => { | |||
| status={item.avatarProps.status} | |||
| badge={item.avatarProps.badge} | |||
| surfaceColor={ | |||
| highlightedValue === item.value ? 'background.primary' : 'background.raised' | |||
| highlightedValue === item.value | |||
| ? 'background.primaryEmphasis' | |||
There was a problem hiding this comment.
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.
Or maybe getColor({ theme, variable: 'background.primaryEmphasis', transparency: 100 }) is a more successful match 🤷
There was a problem hiding this comment.
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
| @@ -29,7 +29,7 @@ export interface IAvatarProps extends HTMLAttributes<HTMLElement> { | |||
| /** | |||
| * 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.
| * `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