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

Fix removeToastNotification to use previousState #5069

Merged
merged 3 commits into from
Sep 20, 2024

Conversation

aaronknol
Copy link
Contributor

@aaronknol aaronknol commented Sep 20, 2024

Important: Request PR reviews on Slack

Please reach out to the design system team on Slack in #prod_design_system for PR reviews. GitHub notifications (e.g. from tagging a person) are not actively monitored.

Why

When we had multiple toast's on a page, all with different id's, calling removeToastNotification would remove all toast's after the one that was being deleted.
The toast's all have a button inside them that when clicked will call removeToastNotification.
This is a similar problem to what we were having with addToastNotification which we fixed here

What

New

By using the previousState syntax of setState, the notifications in the removeToastNotification is always updated and correct.

setNotifications(prev => prev.filter(({ id }) => id !== notificationID))

Old

It appears that the copy of the state that was being created wasn't correct, it hadn't been updated yet before being sliced.

const copy = notifications.slice()
    copy.splice(notificationIndex, 1) // Mutation
    setNotifications(copy)

Copy link

changeset-bot bot commented Sep 20, 2024

🦋 Changeset detected

Latest commit: 7af4bf3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@kaizen/components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vietanhtran16 vietanhtran16 marked this pull request as ready for review September 20, 2024 03:11
@vietanhtran16 vietanhtran16 requested a review from a team as a code owner September 20, 2024 03:11
Copy link
Contributor

github-actions bot commented Sep 20, 2024

✨ Here is your branch preview! ✨

Last updated for commit 7af4bf3: Merge branch 'main' into remove-toast-notification

Copy link
Contributor

@mcwinter07 mcwinter07 left a comment

Choose a reason for hiding this comment

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

Nice, it even looks cleaner than the original method of splicing it out 👍 Cheers for the contribution @aaronknol

@mcwinter07 mcwinter07 merged commit 61448a1 into main Sep 20, 2024
19 checks passed
@mcwinter07 mcwinter07 deleted the remove-toast-notification branch September 20, 2024 05:26
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.

2 participants