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

Handles multiple Keplr popup notifications simultaneously #58

Merged

Conversation

MANTRArob
Copy link

@MANTRArob MANTRArob commented Jan 20, 2025

Motivation and context

This handles the situation where Keplr popup displays two notifications simultaneously.
Typically, we expect that after one notification is accepted or approved, the popup should disappear without showing any further notifications.

This is most likely due to Keplr not having the info of the chain.

Does it fix any issue?

#(issue)

Other useful info

To test this, you could point the test to https://elaisee.mantrachain.io/ which uses MANTRA chain which is not available in Keplr 0.12.68 that the current dev branch is using.

Quality checklist

  • I have performed a self-review of my code.

⚠️👆 Delete any section you see irrelevant before submitting the pull request 👆⚠️

@MANTRArob MANTRArob marked this pull request as ready for review January 22, 2025 02:42
@rabi-siddique rabi-siddique self-requested a review January 30, 2025 03:45
@rabi-siddique
Copy link
Collaborator

@MANTRArob This looks good to me. Could we add a test for this change? It would also be helpful to include instructions for manual testing. Thank you!

@MANTRArob
Copy link
Author

@rabi-siddique Could you give brief instructions on where to add these two items? Thanks.

@rabi-siddique
Copy link
Collaborator

rabi-siddique commented Jan 30, 2025

@rabi-siddique Could you give brief instructions on where to add these two items? Thanks.

@MANTRArob You can add instructions for manual testing in the PR description.
For automated testing, our end-to-end tests are located in the tests/e2e/specs/keplr/keplr-spec.js file. You can add tests there or create a new file in the same directory if needed.

…or the notification to be dismissed to handled situation when 2 notifications are showing
@MANTRArob MANTRArob force-pushed the add_double_notifications_handling branch from e8609b8 to df1b278 Compare February 19, 2025 06:53
@MANTRArob
Copy link
Author

@rabi-siddique I added a new file to show that this method would still work with the existing flow defined in tests/e2e/specs/keplr/keplr-spec.js

@MANTRArob
Copy link
Author

@rabi-siddique Removed unnecessary tests. I ran it locally against the sdk demo build

@rabi-siddique
Copy link
Collaborator

Thank You @MANTRArob!

@rabi-siddique rabi-siddique merged commit 66217e3 into agoric-labs:dev Feb 24, 2025
1 check 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.

2 participants