Skip to content

Conversation

@jzempel
Copy link
Member

@jzempel jzempel commented Dec 2, 2024

Description

While working on a project with Garden dependencies, I realized that react-chrome fails to install in certain environments due to a missing react-uid dependency. This is often masked, since react-notifications pulls it in. Since Garden no longer recommends react-uid in favor of the more stable useId from @zendeskgarden/container-utilities, I updated the missing dependency and code accordingly (eventually, we'll also replace uid in notifications useToast).

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

@jzempel jzempel requested a review from a team as a code owner December 2, 2024 18:42
Comment on lines +88 to +89
/** Identifies the sheet */
id?: string;
Copy link
Member Author

Choose a reason for hiding this comment

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

Even though it is a repeat from the extends, Garden documents id when there are meaningful relationships that result when provided by the consumer (compare PaneProvider).

@coveralls
Copy link

Coverage Status

coverage: 95.656% (-0.001%) from 95.657%
when pulling 719f613 on jzempel/chrome-dependencies
into 2c2c590 on main.

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.

Nice catch! 💯

@jzempel jzempel merged commit 2aba142 into main Dec 2, 2024
9 checks passed
@jzempel jzempel deleted the jzempel/chrome-dependencies branch December 2, 2024 19:48
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.

4 participants