Skip to content

Conversation

@ze-flo
Copy link
Contributor

@ze-flo ze-flo commented Jun 18, 2024

Description

Part 2 of the Modals recoloring effort.

Detail

  • Adds light / dark mode colors to TooltipModal and Drawer
  • Fix Modal shadows to align with latest wires

Screenshot 2024-06-18 at 6 02 31 AM

Screenshot 2024-06-20 at 9 10 07 AM

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

'data-garden-id': COMPONENT_ID,
'data-garden-version': PACKAGE_VERSION
'data-garden-version': PACKAGE_VERSION,
size: 'small'
Copy link
Member

Choose a reason for hiding this comment

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

[nit] the general Garden preference is to locate these props on the element component – where they provide somewhat better prominence. Styled components that show otherwise are anti-patterns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that what you expected?

e334bef

Copy link
Member

Choose a reason for hiding this comment

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

Yes 👍. Matches @geotrev's treatment in #1842

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.

Everything looks great!

Just one thing: the story for TooltipModal is using getColorV8 which makes the trigger text dim in dark mode:

Screenshot 2024-06-20 at 1 46 58 PM

foreground.default oughta do it?

@ze-flo
Copy link
Contributor Author

ze-flo commented Jun 20, 2024

Everything looks great!

Just one thing: the story for TooltipModal is using getColorV8 which makes the trigger text dim in dark mode:

Screenshot 2024-06-20 at 1 46 58 PM `foreground.default` oughta do it?

Good call out. 💯 This is related to what @jzempel described during our Design sync. I'll explicitly define the foregroundColor for now. Once Avatar gets the "recolor" treatment, we might be able to use its default foreground color instead.

9b3e545

Copy link

@steue steue left a comment

Choose a reason for hiding this comment

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

🚀 LGTM!

Copy link

@lucijanblagonic lucijanblagonic left a comment

Choose a reason for hiding this comment

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

Looks great! 🙌

@ze-flo ze-flo merged commit ee0e4f5 into next Jun 21, 2024
@ze-flo ze-flo deleted the ze-flo/modals-recolor-2 branch June 21, 2024 18:52
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.

7 participants