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

Convert the Snackbar component to TypeScript #45472

Merged
merged 41 commits into from Nov 16, 2022

Conversation

kienstra
Copy link
Contributor

@kienstra kienstra commented Nov 1, 2022

What?

Convert the Snackbar component to TypeScript

Why?

As part of an effort to convert @wordpress/components to TypeScript

How?

Mainly by adding types to the Snackbar and SnackbarList components

Testing Instructions

  1. npm run storybook:dev
  2. Expected: The Storybook still works

Screenshots or screencast

Screen Shot 2022-11-01 at 3 56 43 PM

Follow-up

  • SnackbarList is not very well-documented (it doesn't even have a README) and prior to this PR, didn't have Storybook examples either. We should re-organize the folder structure, improve the docs and come up with a better Storybook strategy (e.g. use subcomponents and merge Snackbar and SnackbarList in the same file)
  • We should refactor the Notice component to TypeScript and re-use its types in Snackbar
  • I came across a framer-motion bug when clicking rapidly on two Snackbars in a list — the second Snackbar goes transparent but doesn't get fully removed
  • The timeout logic for Snackbar to be dismissed works in a bit of an unexpected way (snackbar items don't get dismissed in order
  • Take isDismissible prop from notice data is not taken into account by Snackbar (although this could be on purpose)
  • The Snackbar icon (passed through its icon prop) is not vertically centered, doesn't seem to work well with WordPress icons

@codesandbox
Copy link

codesandbox bot commented Nov 1, 2022

CodeSandbox logoCodeSandbox logo  Open in CodeSandbox Web Editor | VS Code | VS Code Insiders

@kienstra kienstra marked this pull request as ready for review November 1, 2022 22:47
@kienstra
Copy link
Contributor Author

kienstra commented Nov 2, 2022

Hi @ciampo,
Would you have time to review this? Thanks so much for helping me with the ColorPalette one.

Thanks, Marco!

@ciampo ciampo added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Nov 2, 2022
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

THank you @kienstra for your work here!

I've left a few comments, I'll take another look at the code (including Storybook) once they've been addressed

Let's leave the work on the README last, so that we can sync it with whatever updates have been made to the component.

packages/components/src/snackbar/README.md Show resolved Hide resolved
packages/components/src/snackbar/types.ts Outdated Show resolved Hide resolved
packages/components/src/snackbar/types.ts Show resolved Hide resolved
packages/components/src/snackbar/list.tsx Outdated Show resolved Hide resolved
packages/components/src/snackbar/types.ts Outdated Show resolved Hide resolved
packages/components/src/snackbar/index.tsx Outdated Show resolved Hide resolved
packages/components/src/snackbar/types.ts Outdated Show resolved Hide resolved
packages/components/src/snackbar/types.ts Outdated Show resolved Hide resolved
packages/components/src/snackbar/list.tsx Outdated Show resolved Hide resolved
packages/components/src/snackbar/types.ts Outdated Show resolved Hide resolved
@mirka mirka added this to In progress (owned) ⏳ in WordPress Components via automation Nov 2, 2022
@kienstra
Copy link
Contributor Author

kienstra commented Nov 2, 2022

Thanks a lot for reviewing this, Marco! I'll apply your suggestions today.

@kienstra
Copy link
Contributor Author

kienstra commented Nov 3, 2022

Hi @ciampo,
Thanks for your ideas and code. Just finished with your suggestions.

@kienstra
Copy link
Contributor Author

kienstra commented Nov 4, 2022

Also, I'll be out until Tuesday, November 8th.

@kienstra
Copy link
Contributor Author

Hi @ciampo,
Would you have time for another round of review?

Thanks for your huge amount of patience on my PRs 😄

@ciampo
Copy link
Contributor

ciampo commented Nov 15, 2022

Hey @kienstra , I've been partially AFK last week and will be a bit slow to respond this week too — but I'll definitely get back to you!

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

Just a few tweaks, and then we're good to merge! Thank you 🙏

packages/components/src/snackbar/README.md Outdated Show resolved Hide resolved
packages/components/src/snackbar/README.md Outdated Show resolved Hide resolved
packages/components/src/snackbar/index.tsx Outdated Show resolved Hide resolved
@kienstra
Copy link
Contributor Author

Thanks, @ciampo! Just applied your suggestions.

No hurry responding, thanks for responding when you're AFK.

Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Thank you for your work on this one!

@ciampo ciampo merged commit 1a2f1ab into WordPress:trunk Nov 16, 2022
WordPress Components automation moved this from In progress (owned) ⏳ to Done 🎉 Nov 16, 2022
@github-actions github-actions bot added this to the Gutenberg 14.7 milestone Nov 16, 2022
@kienstra
Copy link
Contributor Author

Thanks so much, Marco! You're so patient.

@kienstra kienstra deleted the update/snackbar-to-ts branch November 16, 2022 16:17
@ciampo ciampo mentioned this pull request Nov 16, 2022
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants