-
Notifications
You must be signed in to change notification settings - Fork 365
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
refactor: [M3-7960] - Update Notistack to 3.0.1
#10357
Conversation
3.0.1
@@ -132,7 +132,7 @@ export const PlacementGroupsAssignLinodesDrawer = ( | |||
? error[0].reason | |||
: 'An error occurred while adding the Linode to the group' | |||
); | |||
enqueueSnackbar(error[0]?.reason, { variant: 'error' }); | |||
enqueueSnackbar(error, { variant: 'error' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only test to complain from running the suite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it safe to pass an APIError[]
to enqueueSnackbar
? I'm worried this could cause a crash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely not safe 🤦
fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually it takes React Node
as type, which is why is accepted an array. Need to figure out what is does with it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, so yeah, since catch error
is any
it'll take it.
That being said I tested it and it won't throw with ApiError (just an empty toast, good to know I guess), but it isn't type safe
enqueueSnackbar([{ field: 'label', reason: 'Label is too long' }], {
variant,
});
@@ -8,7 +8,7 @@ import { APIError } from '@linode/api-v4/lib/types'; | |||
import { vpcsValidateIP } from '@linode/validation'; | |||
import { CreateLinodeSchema } from '@linode/validation/lib/linodes.schema'; | |||
import Grid from '@mui/material/Unstable_Grid2'; | |||
import { WithSnackbarProps, withSnackbar } from 'notistack'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
withSnackbar
HOC has been deprecated in favor of direct imports.
In short:
- direct imports for class components
useSnackbar
for functional components
CC @bnussman-akamai for Linode Create v2, tho I guess you handle this already the right way, or will
Coverage Report: ❌ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me! ✅
Would like to confirm e2es pass before approving
@bnussman-akamai no luck in getting a run to run at all, getting Will need help from @jdamore-linode |
'[aria-describedby="notistack-snackbar"]', | ||
message, | ||
options | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bnussman-akamai i went with using an existing attribute as a CY selector instead, removing the need to pass a non type-safe one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! looks good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ✅
Tested snackbars and general functionality in several places throughout the app ✅
This reverts commit 196167d.
Description 📝
Update Notistack to 3.0.1 - pretty light refactor for a major update. This will bring us up to date with the lib and prevent some console errors we saw surface recently between React/Router & Notistack. ex:
Every time we'd have a snackbar pop up ⬆️
see Migration Notes for more info
Changes 🔄
3.0.1
Target release date 🗓️
Please specify a release date to guarantee timely review of this PR. If exact date is not known, please approximate and update it as needed.
Preview 📷
ℹ️ There should be no functional or visual regressions as a result of this PR (1/1 refactor)
How to test 🧪
Verification steps
yarn && yarn up
As an Author I have considered 🤔
Check all that apply