Skip to content

Conversation

jzempel
Copy link
Member

@jzempel jzempel commented Aug 14, 2024

Description

Updates related prop documentation and sets the following props to be able to receive color variable key strings:

  • Timeline.Item surfaceColor
  • Avatar
    • backgroundColor
    • foregroundColor
    • surfaceColor
  • Span hue

Detail

Tag is a description update only as the primary hue colors already received intended light/dark treatment.

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 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

@jzempel jzempel requested a review from a team as a code owner August 14, 2024 20:28
Copy link
Contributor

@geotrev geotrev left a comment

Choose a reason for hiding this comment

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

Looks good to me. One comment mainly but it's non-blocking.

}

if ($foregroundColor) {
foregroundColor = $foregroundColor.includes('.')
Copy link
Contributor

@geotrev geotrev Aug 14, 2024

Choose a reason for hiding this comment

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

This feels like it might be overkill, but would we benefit from a utility function from the theming package?

const parseColorProp = (value) => value.includes('.') ? getColor({ theme, variable: value }) : value

// in component:
const foregroundColor = parseColorProp($foregroundColor)

}

return css`
transition: color 0.1s ease-in-out;
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉

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.

Well done. Big win for our consumers! 🔥 🚀

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