Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Fix change or disconnect wallet after claim confirm #2337

Merged
merged 8 commits into from
Jan 31, 2022

Conversation

nenadV91
Copy link
Contributor

Summary

Fixes #2251


// handle account disconnect or account change after claim is confirmed
useEffect(() => {
if (!account || (account !== previousAccount && claimStatus === ClaimStatus.CONFIRMED)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't the useEffect trigger on account change? why is previousAccount needed? I would think

useEffect(() => {
    if (!account || claimStatus === ClaimStatus.CONFIRMED) {
...

should work, it doesn't?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since claimStatus is dependency this (!account || claimStatus === ClaimStatus.CONFIRMED) would activate instantly when the status becomes confirmed and you would not see claim success view at all. So instead this account !== previousAccount && claimStatus === ClaimStatus.CONFIRMED this will only activate when the account changes and the claim is confirmed.

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@alfetopito
Copy link
Contributor

I thought this was fixed in #2322, no?

@nenadV91
Copy link
Contributor Author

nenadV91 commented Jan 28, 2022

@alfetopito Just checked on release branch, looks like its still there when you disconnect the wallet after claim is confirmed you see this
Screenshot from 2022-01-28 01-14-28

In that PR it doesn't reset the activeClaimedAccount because of !isSearchUsed on this line

if (!isSearchUsed && account) {

@elena-zh
Copy link

Hey @nenadV91 , changes LGTM!
However, would it be better to navigate a user to this screen when change accounts
image
instead of this one?
image

The same is for the flow when open another page and then go back to the Claim page.

@nenadV91
Copy link
Contributor Author

@elena-zh Should be all there now.

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Great!

@nenadV91 nenadV91 merged commit 35cdf27 into release/1.10 Jan 31, 2022
@alfetopito alfetopito deleted the 2251/fix-claimed-on-disconnect branch February 1, 2022 17:46
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants