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

Claimed amount on success screen #2253

Merged
merged 6 commits into from
Jan 24, 2022
Merged

Conversation

alfetopito
Copy link
Contributor

Summary

Show claimed amount on success screen:

Screen Shot 2022-01-21 at 13 44 46
Screen Shot 2022-01-21 at 13 45 08

  • Claimed amount is stored on redux as a locale aware formatted amount string

  • It's saved after the claim callback returns (after user sends tx)

    To Test

  1. Make a claim
  • Observe claimed amount of vCow is displayed while claiming and on success screen

@alfetopito alfetopito self-assigned this Jan 21, 2022
@alfetopito alfetopito requested review from a team January 21, 2022 22:11
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@fairlighteth
Copy link
Contributor

Nice looks good! Will style further in upcoming PRs.

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.

Should we include also the amount while you are signing the transaction?

image

@alfetopito
Copy link
Contributor Author

Should we include also the amount while you are signing the transaction?

image

Due to lazyness and simplicity I decided not to.
Simplicity because I can re-use the calculation done on the claim callback and not have to re-do it everywhere else.

And because of that I don't have the amount until the tx is sent.

I could pass a callback to the claim callback but that does not sound like a good idea.

The alternative is to repeat the calculation outside of the claim callback.

I can do that if you think this is a must, but I don't think it's a big deal not having it until the tx is sent

@anxolin
Copy link
Contributor

anxolin commented Jan 23, 2022

I can do that if you think this is a must, but I don't think it's a big deal not having it until the tx is sent

Not that bad. I think is nice to see while you sign it, but imo we can live without it

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.

Seems good!

@elena-zh
Copy link

LGTM!

@alfetopito alfetopito merged commit f4ae160 into develop Jan 24, 2022
@alfetopito alfetopito deleted the claimed-amount-on-success-screen branch January 24, 2022 14:58
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.

5 participants