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 duplicate notifications when switching chains #146

Merged

Conversation

garyghayrat
Copy link
Member

@garyghayrat garyghayrat commented Nov 21, 2023

Before:

Screen.Recording.2023-11-22.at.2.04.09.PM.mov

After:

Screen.Recording.2023-11-22.at.2.02.41.PM.mov

closes #103

Copy link

netlify bot commented Nov 21, 2023

Deploy Preview for l2-flexible-voting ready!

Name Link
🔨 Latest commit 92b5bff
🔍 Latest deploy log https://app.netlify.com/sites/l2-flexible-voting/deploys/656a44bcdcccd70008935504
😎 Deploy Preview https://deploy-preview-146--l2-flexible-voting.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@garyghayrat
Copy link
Member Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

console.log('notify success');
setIsNotificationShownAlready(true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This works for the first bridge, but if I try to bridge twice than my success toast doesn't get shown. We have to reset the isNotificationShownAlready state when a new toast is fired.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed here: 7326eda

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you think the overall approach of this PR is good and can be taken out of Draft?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Good question! After working on the notifications, I think it may be better if we handle this in the NotificationProvider. In this function. If the notification id is the same we shouldn't add it to setNotifications

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, exactly!

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome, thanks! will take out of draft now.

@garyghayrat garyghayrat force-pushed the 11-21-Fix_duplicate_notifications_when_switching_chains branch from 7326eda to eb8a56e Compare December 1, 2023 20:14
@garyghayrat garyghayrat marked this pull request as ready for review December 1, 2023 20:59
@alexkeating alexkeating merged commit 9cff14d into main Dec 4, 2023
6 checks passed
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.

Notifications: Weird change network behavior creates duplicate notifications
2 participants