feat(buttons): new light/dark mode colors #1807
Conversation
| } | ||
| const disabledBackgroundColor = getColor({ theme, variable: 'background.disabled' }); | ||
| const disabledForegroundColor = getColor({ theme, variable: 'foreground.disabled' }); | ||
| const offset100 = { dark: { offset: -100 }, light: { offset: +100 } }; |
There was a problem hiding this comment.
Do we need the unary + operator?
| const offset100 = { dark: { offset: -100 }, light: { offset: +100 } }; | |
| const offset100 = { dark: { offset: -100 }, light: { offset: 100 } }; |
There was a problem hiding this comment.
Agreed - shouldn't be necessary if the base operation in theming is just + offset - the math works itself out 😁
There was a problem hiding this comment.
I was preferring the unary for the sake of offset clarity and scannability. But if this feels odd to the team, I'll remove.
| /* | ||
| * Anchor / link button styling | ||
| */ | ||
| const options = { theme, variable: `foreground.${isDanger ? 'danger' : 'primary'}` }; |
There was a problem hiding this comment.
While elegant, this pattern might complicate searching for variable names. For ex, searching for all instances of foreground.primary.
| const options = { theme, variable: `foreground.${isDanger ? 'danger' : 'primary'}` }; | |
| const options = { theme, variable: isDanger ? 'foreground.isDanger' : 'foreground.primary' }; |
There was a problem hiding this comment.
I think I agree with this as well... we'll also probably thank ourselves by helping the vars be searchable.
jzempel
changed the title
| export const StyledIconButton = styled(StyledButton as 'button').attrs(props => ({ | ||
| 'data-garden-id': (props as any)['data-garden-id'] || COMPONENT_ID, |
There was a problem hiding this comment.
I dug further into #1804 and found some regressions with this method. I'll share my findings and recommendation in that PR. Meanwhile, here's a summary.
Notice how IconButton now inherits the COMPONENT_ID buttons.button from StyledButton.
When extending a styled-component with attrs already set, the props will include such attributes. In this case props['data-garden-id'] === 'buttons.button' and takes precedence over the StyledIconButton's COMPONENT_ID.
One way around that is to first check that the data-garden-id exist and doesn't match the id from the styled-component being extended:
| export const StyledIconButton = styled(StyledButton as 'button').attrs(props => ({ | |
| 'data-garden-id': (props as any)['data-garden-id'] || COMPONENT_ID, | |
| export const StyledIconButton = styled(StyledButton as 'button').attrs(props => ({ | |
| 'data-garden-id': (props as any)['data-garden-id'] && (props as any)['data-garden-id'] !== 'buttons.button' ? | |
| (props as any)['data-garden-id'] : COMPONENT_ID, |
I can't recommend this pattern. It's brittle and error-prone. My hunch is that we should set data-garden-id on the components we expose to the public and only rely on attrs for data-garden-version or other immutable attributes.
I'm happy to discuss further.
There was a problem hiding this comment.
I can't recommend this pattern.
Thanks for the careful review. I would say that, within the context of a package, this 2nd pattern (although somewhat awkward) is totally acceptable for now. Eventually, I agree we should probably move data-garden-id out of all view and into element-level components.
There was a problem hiding this comment.
I've reverted the changes here (and updated PR title, description, and tags) in favor of reviewing #1804
|
Everything else look good, colors is correctly applied based on all the props tested. 🙌 Unrelated to the new palette and semantic variables, I see some weird pixel glitching for |
jzempel
changed the title
Description
...rooted in semantic variables.
Detail
Verified stateful (default, hover, active, focused) color treatment for the following in both light and dark modes – with icons enabled wherever applicable:
<Anchor><Anchor isDanger><Button isLink><Button isLink isNeutral>(no change)<Button isLink isDanger><Button isLink isDanger focusInset><Button isLink disabled><Button isBasic><Button isBasic isNeutral><Button isBasic isNeutral focusInset><Button isBasic isDanger><Button isBasic isDanger focusInset><Button isBasic disabled><Button isPrimary><Button isPrimary isNeutral><Button isPrimary isNeutral focusInset><Button isPrimary isDanger><Button isPrimary isDanger focusInset><Button isPrimary disabled><Button><Button isNeutral><Button isNeutral focusInset><Button isDanger><Button isDanger focusInset><Button disabled><SplitButton>isBasicisBasic isNeutralisBasic isDangerisPrimaryisPrimary isNeutralisPrimary isDangerisNeutralisDangerdisabled<ToggleButton><TogglButton isPressed>Checklist
npm start)renders as expected with reversed (RTL) direction?bedrock)tested for WCAG 2.1 AA accessibility compliancetested in Chrome, Firefox, Safari, and Edge