Skip to content
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

fix(theming): return hex from generated getColor values #1999

Merged
merged 5 commits into from
Jan 10, 2025

Conversation

jzempel
Copy link
Member

@jzempel jzempel commented Jan 7, 2025

Description

While this PR focuses on a very minor getColor fix, the main enhancement is the updated Storybook that shows the potential generated palette hue for any non-themed color value resolved by getColor.

Checklist

  • 👌 design updates will be Garden Designer approved (add the designer as a reviewer)
  • 🌐 demo is up-to-date (npm start)
  • ⬅️ renders as expected with reversed (RTL) direction
  • ⚫ renders as expected in dark mode
  • 🤘 renders as expected with Bedrock CSS (?bedrock)
  • 💂‍♂️ includes new unit tests. Maintain existing coverage (always >= 96%)
  • tested for WCAG 2.1 AA accessibility compliance
  • 📝 tested in Chrome, Firefox, Safari, and Edge

@coveralls
Copy link

coveralls commented Jan 7, 2025

Coverage Status

coverage: 95.536%. remained the same
when pulling 0612cce on jzempel/getcolor-shades
into 919df82 on main.

<LG style={{ marginTop: 20 }}>Generated hue</LG>
<MD style={{ marginBottom: 12 }}>
<Span hue="foreground.subtle">
Not used when <Code>hue[shade]</Code> is defined by the theme{' '}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the output from passing neutralHue as a hue arg.

Screenshot 2025-01-08 at 3 04 25 PM

Even though it's a recognized hue/shade combo, the generated color scale deviates from our palette.

const grey {
    100: '#f8f9f9',
    200: '#e8eaec',
    300: '#d8dcde',
    400: '#b0b8be',
    500: '#919ca5',
    600: '#848f99',
    700: '#5c6970',
    800: '#39434b',
    900: '#293239',
    1000: '#1c2227',
    1100: '#151a1e',
    1200: '#0a0d0e'
}

One who isn't familiar with the inner workings of getColor could have expected a match.

Instead, should we only show the generated palette when all 3 conditions below are true?

  1. variable is undefined
  2. hue doesn't match any of the keys (excluding variables) from theme.colors
  3. hue doesn't match any of the keys (excluding product) from theme.palette

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started with this approach, and then got lazy with the Story 😜. I'll go back and refine.

<StyledDiv $background={background}>{tag}</StyledDiv>
{!!generatedHue && (
<>
<LG style={{ margin: '20px 0 12px' }}>Generated hue</LG>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Should we align our vocabulary with the Color Principles docs?

Suggested change
<LG style={{ margin: '20px 0 12px' }}>Generated hue</LG>
<LG style={{ margin: '20px 0 12px' }}>Generated palette</LG>

Alternatively:

Suggested change
<LG style={{ margin: '20px 0 12px' }}>Generated hue</LG>
<LG style={{ margin: '20px 0 12px' }}>Generated color scale</LG>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to align with the technical theme object and palette documentation. Btw, the term is also used in color principles.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see. 👍

Copy link
Contributor

@ze-flo ze-flo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great enhancement! 💯

@jzempel jzempel merged commit 300f5f9 into main Jan 10, 2025
8 checks passed
@jzempel jzempel deleted the jzempel/getcolor-shades branch January 10, 2025 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants