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

feat: add refresh address button for qr code #692

Merged

Conversation

jpyepez
Copy link
Contributor

@jpyepez jpyepez commented Mar 10, 2024

Description

This PR addresses issue #431

  • Add Refresh Address button to receive on-chain dialog
  • Add loading and error handling during refresh (using previously exisiting SetStateAction functions)
  • Add related unit tests
  • Add translations (to the best of my ability, happy to remove if this is not needed)

Screen Recording

This is a short demo of the resulting behaviour (with an overridden response, given that the mock backend seems to return the same address from the new-address endpoint).

refresh_address

@cstenglein cstenglein linked an issue Mar 10, 2024 that may be closed by this pull request
@cstenglein
Copy link
Collaborator

Awesome, thank you very much!

),
);

const refreshBtn = await screen.findByRole("button", {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like the test is failing - could you check that please? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @cstenglein , no worries at all. Happy to contribute, and hopefully there'll be a chance to keep helping out.

Interesting, I'm checking this one out. I noticed that there's an uncaught request going out, and I managed to catch it with a beforeEach instead of beforeAll at the beginning of these tests. However, it seems like the error is an "unknown login error" which does not seem to be happening when running the tests locally. Can you think of any reasons why that could be?

In any case, let's maybe see if this takes care of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, it seems to have done the trick, right?

Let me know if there's anything else and I'll check it out. 🙂

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the reason is the useEffect hook in the ReceiveOnChain component, which fetches the new address at the start.

Looking good, merging, thank you!

@jpyepez jpyepez force-pushed the feat/add-refresh-button-for-qr-code-431 branch from da8d65a to 9476ac7 Compare March 10, 2024 22:14
),
);

const refreshBtn = await screen.findByRole("button", {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, the reason is the useEffect hook in the ReceiveOnChain component, which fetches the new address at the start.

Looking good, merging, thank you!

@cstenglein cstenglein merged commit 346c145 into raspiblitz:master Mar 12, 2024
2 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.

(Receive) Add refresh Button for QR Code
2 participants