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

[ABA 🎵 Impact] - Fallback message and Loader #1916

Merged
merged 22 commits into from
Dec 3, 2021

Conversation

W3stside
Copy link
Contributor

@W3stside W3stside commented Nov 25, 2021

Summary

Closes #1908

Description && screenshots in #1908

TODO: there are some tweaks to make:

  1. better hiding/showing of message based on price impact - idea will be to (in another PR) make a global flag for ABA impact loading/not existing and wait for that to return before showing/hiding message done using fallback price impact error checking
  2. a loader potentiatlly? done loader in commit

LOADER screenshot

Screenshot 2021-11-29 at 7 50 32 PM

To Test

  1. any chain
  2. any tokens
  3. enter amounts
    4a. see loader in price impact spot
  4. wait for message to appear (if selected token pair has no impact)
  5. switch to other token with known impact
  6. message warning should disappear

@W3stside W3stside marked this pull request as draft November 25, 2021 10:45
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

@W3stside W3stside marked this pull request as ready for review November 25, 2021 13:16
@W3stside W3stside requested review from a team November 25, 2021 13:16
@MareenG
Copy link

MareenG commented Nov 25, 2021

Hey @W3stside,

In generel it works and the messages shows up and disappear. For 1Inch and stake it didn't work I got the error loading price messages but for other tokens it worked as expected.

However, I noticed that the error messages always shows up if you insert new values see video:

Bildschirmaufnahme.2021-11-25.um.14.22.19.mov

Also it doesn't quite work if you already have inserted a high value in the token amount field.
Example
Choose WXDAI and AGVE insert a high switch amount and then change to 1Inch the message is not displayed

Bildschirmaufnahme.2021-11-25.um.14.23.39.mov

@elena-zh
Copy link

elena-zh commented Nov 25, 2021

Hey @W3stside , I'd like to add some cases:

  1. Swap button is disabled to me when I select WXDAI-STAKE token pair.
    disabled
    It appeared, that a token pair does not matter: I always see Swap button disabled: https://watch.screencastify.com/v/L9yyfWSVuDdW99idTSBK
  2. 'unknown' looks like a link to me, and intuitionally I want to click on it.. Is there any possibility to write 'unknown' in bold or so?
    seems like a link

@elena-zh
Copy link

Should we show this warning when we select XDAI/ETH as a token to trade?
image

Currently, we have a task not to show fee warning when these tokens are selected #1853

@elena-zh
Copy link

It would be nice to show this warning a bit nicer in a mobile devices. See the image (iPhone 12 mini): dash is shifted on the 2nd row, looks weird
image

@W3stside W3stside changed the title [ABA 🎵 Impact] - Fallback message on !priceImpact [ABA 🎵 Impact] - Fallback message and Loader Nov 29, 2021
@alfetopito
Copy link
Contributor

Tested only xDai

Looking great, loader behaving fine and giving the warning on unknown estimations like the infamous 1inch
Screenshot from 2021-11-29 11-09-11

But I did get a weird USD estimation (although amounts look ok) for DAI
Screenshot from 2021-11-29 11-14-30

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.

I love the loading, i think is a great solution!

I would delay the appearance fo the loadera bit maybe? at least in xDAI was loading fast and making an effect like everything was moving a lot. I was feeling it takes too much attention (maybe good, but i don't like this part a lot).

The numbers are completely weird, or this is out of this PR?
image

image

src/custom/components/SwapWarnings/index.tsx Outdated Show resolved Hide resolved
@W3stside
Copy link
Contributor Author

@elena-zh @anxolin

Screenshot 2021-11-29 at 11 50 51 PM

@W3stside
Copy link
Contributor Author

I love the loading, i think is a great solution!

I would delay the appearance fo the loadera bit maybe? at least in xDAI was loading fast and making an effect like everything was moving a lot. I was feeling it takes too much attention (maybe good, but i don't like this part a lot).

ok something to consider, i'll look into it today

The numbers are completely weird, or this is out of this PR? image

cause of the simple function im using, so in another pr will be fixed

@elena-zh
Copy link

elena-zh commented Nov 30, 2021

Loading looks great to me. However, sometimes it runs too long. In addition, sometimes I am able to get more than a huge % after a loading, but then in a sec the value was changed: https://watch.screencastify.com/v/BiSQvhtOZrFx8AWuu0ZT

But I still see Swap button disabled when there is no price impact warning
image
high price impact

Or these changes are out of scope for this PR?

Again, I still see this issue #1916 (comment) . Please let me know if I need to open a separate issue for this.

Another nitpick, is that there is a different padding between warnings on the Swap form and confirmation modals. It would be good to have the same one
swap
confirm

@elena-zh
Copy link

elena-zh commented Dec 2, 2021

Hey @W3stside , now the 'Swap/ button is enabled, but a bit weird to see Price impact message in the confirmation modal. See the video:
https://watch.screencastify.com/v/E7RbQ4fQqbnYFfK4Vixr

[ABA - fixes] - Fix warning showing when not needed, and show correct impact in modal
[ABA] Final logic check - use SELL > SELL and BUY > SELL
@W3stside W3stside merged commit be9ba64 into aba-test-logic Dec 3, 2021
@W3stside
Copy link
Contributor Author

W3stside commented Dec 3, 2021

@anxolin @gnosis/gp-frontend @gnosis/gp-qa please post merge review if necessary, tho this one is outdated from the others merged in (safe to ignore)

@alfetopito alfetopito deleted the aba-test-fallback-message branch December 3, 2021 18:16
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