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

2473/cancel claim issue #2485

Merged
merged 3 commits into from
Feb 24, 2022
Merged

2473/cancel claim issue #2485

merged 3 commits into from
Feb 24, 2022

Conversation

nenadV91
Copy link
Contributor

@nenadV91 nenadV91 commented Feb 21, 2022

Summary

Fixes #2473

  • fixes the issue with showing success claim view when the transaction is cancelled

Todo

  • Reset claim repo and address after review

@github-actions
Copy link
Contributor

github-actions bot commented Feb 21, 2022

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@nenadV91 nenadV91 marked this pull request as ready for review February 22, 2022 00:22
@nenadV91 nenadV91 requested review from a team February 22, 2022 00:22
@elena-zh
Copy link

elena-zh commented Feb 22, 2022

Hey @nenadV91 , great changes!

One tiny nitpick: when a transaction is cancelled, we get a pop-up notification with a green checkmark on it, so it might seem that the Tx is successful.
Could we change the icon to the red one (like we use for the failed transactions) for this case?
change checkmark to fail

Also, just FYI, I have created a separate issue for the case when a transaction is executed, but it hangs in the 'Cancelling' pending state #2486

Copy link
Contributor

@W3stside W3stside left a comment

Choose a reason for hiding this comment

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

did u also change the contract addreses and branch name?

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

Looks ok to me if it passes Elena tests. Pls, don't merge until you revert the branch name

@@ -62,7 +62,7 @@ import useIsMounted from 'hooks/useIsMounted'
import { ChainId } from '@uniswap/sdk'
import { ClaimInfo } from 'state/claim/reducer'

const CLAIMS_REPO_BRANCH = 'gip-13'
Copy link
Contributor

Choose a reason for hiding this comment

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

Very careful with merging this. If this gets to production we will have some troubles

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.

@nenadV91 , I have reported a separate issue #2490 for the issue I mentioned above as it is not exactly related to this PR.

@nenadV91
Copy link
Contributor Author

@elena-zh Since there is a separate issue for pop-up notification I am merging this one

@nenadV91 nenadV91 merged commit af28821 into develop Feb 24, 2022
@nenadV91 nenadV91 deleted the 2473/cancel-claim-issue branch February 24, 2022 20:19
@github-actions github-actions bot locked and limited conversation to collaborators Feb 24, 2022
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.

Success screen is displayed when cancel a claim TX
4 participants