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

[Debt] Replaces storybook-addon-intl with storybook-react-intl #10022

Merged
merged 3 commits into from
Apr 10, 2024

Conversation

mnigh
Copy link
Contributor

@mnigh mnigh commented Apr 9, 2024

🤖 Resolves #10021.

👋 Introduction

This PR replaces storybook-addon-intl with storybook-react-intl.

🎩 Acknowledgement

@esizer for stopping the infinite re-render caused by useEffect.

🧪 Testing

Assist reviewers with steps they can take to test that the PR does what it says it does.

  1. pnpm storybook
  2. Verify stories can still switch locale

@petertgiles petertgiles self-requested a review April 10, 2024 12:28
Copy link
Contributor

@petertgiles petertgiles left a comment

Choose a reason for hiding this comment

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

It works well! I'm worried that the useEffect fix may have broken some Chromatic snapshotting though.

@@ -33,15 +33,13 @@ type ThemeSetterProps = {
theme: Theme;
};
const ThemeSetter = ({ theme }: ThemeSetterProps) => {
const { setTheme } = useTheme();
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if it's as a result of the library change or of this theme change but it seems like Chromatic is taking some snapshots before the components are ready.

For these, the theme icon hasn't yet settled to the right one for the story:
image

And this one still has the loading spinner running:
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good 👁️, thanks! As discussed, any of the stories with the ThemeSwitcher component were actually previously incorrect due to the infinite re-render bug in ThemeDecorator where the initial state should have been the system default rather than light mode. The home page I've added a Chromatic delay to the DateInput and Pending stories that should get them to where they should be (more or less). The other diffs are existing issues: #9636, #10025.

Copy link
Contributor

Choose a reason for hiding this comment

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

For anyone else who might be confused like me, the switcher is now the globe icon in the toolbar.
image

@mnigh mnigh requested a review from petertgiles April 10, 2024 17:29
@mnigh mnigh added this pull request to the merge queue Apr 10, 2024
Merged via the queue into main with commit 0af61ea Apr 10, 2024
12 checks passed
@mnigh mnigh deleted the 10021-storybook-react-intl branch April 10, 2024 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

♻️ Replace storybook-addon-intl with storybook-react-intl
2 participants