status-im / status-mobile

a free (libre) open source, mobile OS for Ethereum
https://status.app
Mozilla Public License 2.0
3.89k stars 984 forks source link

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

Closed rachelhamlin closed 4 years ago

rachelhamlin commented 5 years ago

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)

StatusSceptre commented 4 years ago

All good @alexjg! Good to know. Hope all's well.

gitcoinbot commented 4 years ago

@alexjg Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

gitcoinbot commented 4 years ago

@alexjg Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

alexjg commented 4 years ago

@gitcoinbot Yup still going. Pushing hard to get a PR in before the weekend.

alexjg commented 4 years ago

Quick update here. I'm working on getting the refreshControl property of the relevant ScrollViews set up in order to build the specced UI. I'm struggling a little to figure out how to pass a react component as a property to a reagent view, which I need to do to pass RefreshControl to the refreshControl property, but I'll figure it out.

In terms of events and applications state I'm thinking to display the loading spinner whenever :prices-loading? is true and to dispatch :wallet-ui.pull-to-refresh when pulling to refresh, hopefully that makes sense.

As a side note I'm running into some issues with the build environment, specifically I have to completely restart the application if I run into any errors. Additionally, tapping on a yellow error box causes the application to crash, so not only do I have to restart the application but I can't easily get access to compile errors. I'll chase this up in other avenues though.

StatusSceptre commented 4 years ago

Hey @alexjg - do you need any help trouble shooting your build env?

alexjg commented 4 years ago

Hey @StatusSceptre possibly yeah. Should I create issues on the bug tracker or ask for advice in discuss.status.im?

alexjg commented 4 years ago

I'm working around it at the moment. It's not slowing me down too much as I'm mostly stuck on how to get the refreshControl property to actually do anything. I've successfully passed the RefreshControl to the ScrollView using reagent/as-element but it's not doing anything useful (ScrollView just displays blank). I'm still troubleshooting that but am beginning to wonder if I'll need to write the pull down to refresh functionality from scratch.

alexjg commented 4 years ago

I've managed to get RefreshControl working but it doesn't really give us the hooks we need. It renders the spinner on top of the scrollview content in the z axis and doesn't really expose any way to render the spinner elsewhere or slowly transition it in as the user scrolls down. I think we'll need to build the pull to refresh functionality ourselves, so unless there's already something in the codebase to do that (I can't find anything) then I'll make a start on it tomorrow.

StatusSceptre commented 4 years ago

I'm just going to ping a few people who might be able to help. @alexjg is having trouble with his build environment - @PombeirP @yenda @flexsurfer could one of you possibly help trouble shoot?

flexsurfer commented 4 years ago

@alexjg @rachelhamlin i'm not sure , issue title says "Add loading state for currency conversion" why do we need RefreshControl for it?

flexsurfer commented 4 years ago

As a side note I'm running into some issues with the build environment, specifically I have to completely restart the application if I run into any errors. Additionally, tapping on a yellow error box causes the application to crash, so not only do I have to restart the application but I can't easily get access to compile errors. I'll chase this up in other avenues though.

@alexjg could you provide more details pls better in a new issue with screenshots or video thanks!

alexjg commented 4 years ago

@flexsurfer I've created a new issue here

alexjg commented 4 years ago

@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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@alexjg correct, thanks!

andmironov commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago
hesterbruikman commented 4 years ago

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.

yenda commented 4 years ago

@alexjg you can call update-prices https://github.com/status-im/status-react/blob/88dbeead42f640a1ba8592041bbdcff656297799/src/status_im/wallet/core.cljs#L242

It uses get-prices here https://github.com/status-im/status-react/blob/298f000fcb4bd98b4180f196fcd80c0508696983/src/status_im/utils/prices.cljs#L42 to which you can add a timeout

alexjg commented 4 years ago

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 commented 4 years ago

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

alexjg commented 4 years ago

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

rachelhamlin commented 4 years ago

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

alexjg commented 4 years ago

@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.

gitcoinbot commented 4 years ago

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 commented 4 years ago

@vkjr awesome work!