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

[#8026] Add loading state for currency conversion #8443

Closed
rachelhamlin opened this issue Jun 20, 2019 · 80 comments · Fixed by #10196
Closed

[#8026] Add loading state for currency conversion #8443

rachelhamlin opened this issue Jun 20, 2019 · 80 comments · Fixed by #10196
Assignees
Labels
feature feature requests

Comments

@rachelhamlin
Copy link
Contributor

rachelhamlin commented Jun 20, 2019

Description

Type: Feature

Summary: Continuation of the work done in this PR for #8026.

New overview screen shows fiat values for the total amount + gas. This can sometimes take time to load, so we need to add a loading state.

Expected behavior

Designs found here: preferred implementation, back-up option

When converted currency in the following fields is not yet available, we display a loading state instead.

image
Loaded currencies

image
Preferred design is an in-line animation (see comment)

@hesterbruikman
Copy link
Contributor

hesterbruikman commented Jun 20, 2019

A related issue:

In case of errors, the expected pattern is the "Tooltip error" that has a little tip at the bottom and hovers slightly above the list item that contains the error. In the merged build, the tool misses the tip. It has appeared, in the correct position, in an earlier test build, which is where the image on the right comes from.

cc @andmironov

Frame 2

@andmironov
Copy link

andmironov commented Jun 21, 2019

I'd suggest something like this to indicate no ETH for gas:

  • Using "network fee" rather than "Gas" for consistency;
  • Letting know that you have to have the required amount on the wallet you're sending the transaction from;
  • Providing a helpful call-to-action.

As for the wrong password, I'd use the default tooltip

no-gas

wp

@errorists
Copy link
Contributor

for the loader I'd use an inline spinner

inlineLoader

engineers activityIndicator, color: grey, size: iOS: small Android: 16

@andmironov
Copy link

FIgured it might be better to re-use this thing to dosplay the warning:
no-eth

@hesterbruikman
Copy link
Contributor

hesterbruikman commented Jun 24, 2019

FIgured it might be better to re-use this thing to dosplay the warning:
no-eth

Looks like this should be a new issue to update the 'Not enough ETH'-warning. Love the entry to Receive ETH on there!

@hesterbruikman
Copy link
Contributor

FIgured it might be better to re-use this thing to dosplay the warning:
no-eth

Seperate the loading state update (this issue) from the warning update: #8485

@rachelhamlin rachelhamlin added v1 and removed v1 audit labels Jul 23, 2019
@hesterbruikman
Copy link
Contributor

I believe we also need a loading state for the fiat value shown in the wallet overview and individual accounts. It currently shows '0' until conversion is fetched.

@flexsurfer @rachelhamlin should that be a seprate issue or can it be takn along with this one?
cc @andmironov

@flexsurfer
Copy link
Member

@hesterbruikman can be done here

@hesterbruikman
Copy link
Contributor

@andmironov can you share a design here for loading state of fiat value in Account overview and on Account card?

@hesterbruikman
Copy link
Contributor

@andmironov can you share a design here for loading state of fiat value in Account overview and on Account card?

And also what should happen on refresh. Referring to "Refresh wallet should work like it worked before"

@andmironov
Copy link

andmironov commented Jul 26, 2019

Refresh wallet should work like it worked before

Pull-to-refresh, a spinner is shown while loading:
Screen Shot 2019-07-26 at 13 40 36
Here's an old GIF with the old design, but the concept is the same https://dribbble.com/shots/3777816-Status-Wallet

can you share a design here for loading state of fiat value

Here's what I've had in Figma prepared, made under the impression that it might take a lot of time to load the list of assets and balance:
Screen Shot 2019-07-26 at 13 42 46

If it's not the case, maybe it would be better to go with something like we did in the Dapp Store (shining placeholders):
Screen Shot 2019-07-26 at 13 43 43

up to @flexsurfer to decide which approach fits better technically. Also, the little spinner is used in the transaction overview, which might be another reason not to use the "placeholder" approach

@hesterbruikman
Copy link
Contributor

🙌 thanks @andmironov

@rachelhamlin
Copy link
Contributor Author

I'm about to put a bounty on this and want to clarify-

I believe the error tool tip is already fixed. We're also planning on using a different 'not enough ETH' warning in issue #8485, so I'm going to reduce the scope of this issue to just the inline spinner.

Okay for you @andmironov?

@andmironov
Copy link

andmironov commented Sep 6, 2019

Yes, thanks @rachelhamlin ! The scope of this issue (for the transaction overview modal) I believe boils down to the awesome animation @errorists have made! As a fallback solution I can suggest doing:

Screen Shot 2019-09-06 at 19 51 08

I like the animation more, just being careful if it takes more time/is harder to implement

And the error states will look like this:

Screen Shot 2019-09-06 at 19 55 23

Note: As far as I understand it's part of #8485 but it exceeds that issue's scope a bit as well 🤷‍♂️

@rachelhamlin
Copy link
Contributor Author

rachelhamlin commented Sep 6, 2019 via email

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 0.472 ETH (99.4 USD @ $210.6/ETH) attached to it.

@gitcoinbot
Copy link

gitcoinbot commented Sep 18, 2019

Issue Status: 1. Open 2. Cancelled


Work has been started.

These users each claimed they can complete the work by 5 months ago.
Please review their action plans below:

1) acolytec3 has applied to start work (Funders only: approve worker | reject worker).

I would like to work on this issue following the one of the proposed implementations linked in the bounty issue.
2) rajatkapoor has applied to start work (Funders only: approve worker | reject worker).

I will implement it to exactly match the desired design showing an inline loader
3) alexjg has been approved to start work.

I've enjoyed working with this codebase, but so far I've just been working on styling things. This looks like a chance to get a bit more contact with the reagent subscriptions framework. I have a bit of time this weekend so would love to have a shot at it.
4) morevolk-latei has applied to start work (Funders only: approve worker | reject worker).

I'm good at creating persistent UI's and has a keen eye on frontend design.
I would like to work on this feature and I believe I'm befitting for this.
I have experience in working with React, AngularJs, JS, Node, Django, Linux, etc.
I would love to contribute. :-)

Learn more on the Gitcoin Issue Details page.

@rachelhamlin
Copy link
Contributor Author

Heyo! Sorry for delay on this issue, just got back from our team offsite.

@alexjg I've approved you :) Welcome back! Approved based on experience w/ codebase, quality past contribution and first come, first serve.

@acolytec3 will hopefully be contributing on #8027 instead.

@bshevchenko are you having any better luck getting Status set up after the issue found in #8657? 🤞we can get that fixed.

@rajatkapoor & @morevolk-latei thank you for applying! Do you both have ClojureScript experience? If so, I'm happy to bounty up other UI issues for you to try if interested :)

@bshevchenko
Copy link

@rachelhamlin Pedro answered me back just a few minutes ago on that issue. Trying now what he advised me to do

@alexjg
Copy link
Contributor

alexjg commented Nov 11, 2019

@flexsurfer I've created a new issue here

@alexjg
Copy link
Contributor

alexjg commented Nov 11, 2019

@flexsurfer as regards the RefreshControl, there's been a slight change in scope throughout this issue and the plan is now to implement a pull to refresh interaction for the currency conversion rather than an inline loading state. See this comment

@flexsurfer
Copy link
Member

hey @alexjg i've discussed pull and refresh with @andmironov we can ignore it for this issue, and just show loader indicator when we load balance

@alexjg
Copy link
Contributor

alexjg commented Nov 12, 2019

As in just show the loading spinner at the top of the page (and associated error report) but not implement the pull to refresh?

@flexsurfer
Copy link
Member

@alexjg correct, thanks!

@andmironov
Copy link

andmironov commented Nov 14, 2019

Hey! Just to make things clear, I still think having a pull-to-refresh is important, but it could be ignored for now to keep things simple

just show the loading spinner at the top of the page (and associated error report) but not implement the pull to refresh

that would work for now! I don't know how often the amounts get updated in the background, but if we want to keep things transparent and stay resourceful (Principles!) we still need to show this after a certain timeout:

Screen Shot 2019-11-14 at 12 19 03

@alexjg
Copy link
Contributor

alexjg commented Nov 14, 2019

I'm happy to add that UI to the component I'm building, it's not particularly complicated. However, where's the best place to track that timeout? As part of the component state or is there something in the application state I can use?

@andmironov
Copy link

sorry, I am not familiar with how often the amounts get updated and how long it takes so let's find that out first :-) maybe there's no reason for this state to exist at all

@hesterbruikman
Copy link
Contributor

@hesterbruikman
Copy link
Contributor

how often amounts get updated and how long it takes.

This is a good point. We used to have issues with cryptocompare and a far from pretty loading state would persists, covering the wallet amount. But maybe the conversion pulling works different now.

@alexjg
Copy link
Contributor

alexjg commented Nov 20, 2019

I've been using the prices-loading and prices-update state to track most of this stuff at the moment so adding a timeout shouldn't be too tricky. Most of what I'm doing at the moment is figuring out how the react native animations framework works in a reagent context. I've pretty much got my head around that now so I'm just putting it all together.

@StatusSceptre
Copy link
Member

Hey @alexjg - just a regular check-in. I'm surprised Gitcoinbot hasn't been pinging you! How are things going?

@alexjg
Copy link
Contributor

alexjg commented Dec 19, 2019

Hey! Apologies, this dropped off my radar with the encroachment of christmas. I should have time to complete it over the holidays though.

@rachelhamlin
Copy link
Contributor Author

Cool, no sweat @alexjg. We'll consider you still active :)

@alexjg
Copy link
Contributor

alexjg commented Jan 26, 2020

@rachelhamlin I think realistically I'm not going to have the time to complete this work. I've submitted a PR with the work I've done so far. The loading spinner UI (the layout of the loading notification and animation of the spinner) is done, but I haven't had a chance to complete the logic to animate showing and hiding the loading spinner as the application state changes. It shouldn't be too tough to do, unfortunately I just have too much on my plate to get to it. Apologies for dragging this out, I really wanted to contribute but don't have the bandwitdth right now.

@vkjr vkjr self-assigned this Mar 13, 2020
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Cancelled


The funding of 0.472 ETH (55.27 USD @ $117.09/ETH) attached to this issue has been cancelled by the bounty submitter

@rachelhamlin
Copy link
Contributor Author

@vkjr awesome work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature feature requests
Projects
None yet
Development

Successfully merging a pull request may close this issue.