Refresh control test #19831
Refresh control test #19831
Conversation
Jenkins BuildsClick to see older builds (47)
|
mmilad75
requested review from
J-Son89,
smohamedjavid,
shivekkhurana,
briansztamfater and
ulisesmac
and removed request for
J-Son89
| {:refresh-control (reagent/as-element | ||
| [rn/refresh-control | ||
| {:refreshing (and tokens-loading? init-loaded?) | ||
| :colors colors/neutral-40 |
There was a problem hiding this comment.
I couldn't find the design for dark theme, so I used this for both, and it looked visible and good
| :on-change #(set-selected-tab %)}] | ||
| [tabs/view {:selected-tab selected-tab}]])) | ||
| [rn/scroll-view | ||
| {:refresh-control (reagent/as-element |
There was a problem hiding this comment.
I wonder can we abstract this as-element away?
There was a problem hiding this comment.
I removed it but I got errors. @ulisesmac added this and I kept it.
There was a problem hiding this comment.
Im not saying to remove it, I mean to abstract it away so we don't need to add this method each time we use the pull to refresh, there is many other uses of pull to refresh in the designs so it's nice to take steps like this
There was a problem hiding this comment.
Oh, right. We can create a new refresh-control component and add this there
There was a problem hiding this comment.
nice yeah that sounds good to me!
mmilad75
force-pushed
the
refresh-control-test
branch
from
d4646d7
to
23d10cc
Compare
| @@ -0,0 +1,11 @@ | |||
| (ns quo.components.scroll-view.view | |||
There was a problem hiding this comment.
nit: maybe refreshable-scroll-view?
or add some description of what is different from regular ScrollView
|
I think it is a UX redundancy to show the activity loading indicator and the whole screen loading skeleton while refreshing. IMO the loading skeleton should only be shown the first time because there is no data to show at that time, for refreshing we could just should the activity indicator from refresh control component. |
When trying to re-fetch the accounts using |
Sounds good, maybe we should confirm with the design team if they are okay with this behavior for our release or if we need a follow up issue for prioritize and implement in the future. |
I checked with the design team and they approved it (https://discord.com/channels/624634427930312714/928625369542713396/1235661790109503589). But they asked for replacing the default spinner with the one in the designs. However, @J-Son89 suggested me to skip it since it will take time |
There was a problem hiding this comment.
Looks nice, but VirtualizedLists (FlatLists) should never be nested inside plain ScrollViews.
The component needs to be a FlatList where the assets items are the data and the rest of the components above it are the FlatList header
FYI, when nesting FlatList inside a ScrollView, the FlatList optimizations regarding rendering children will not be applied and will be treated as a ScrollView. (also you should be getting a warning from react-native in metro about this)
The data in the scroll view has different render options |
The problem is not the account cards. We have the ScrollView is vertical and the assets list is vertical. |
@mmilad75 as I discussed with you earlier using a FlatList inside a ScrollView will render the FlatList optimizations useless, consume more RAM, and have a negative effect on the scrolling performance with large lists: https://stackoverflow.com/a/55256341. We plan to make the wallet home our landing page, so let's not rush into a sub-optimal solution. We can discuss this further with other devs. |
| @@ -0,0 +1,11 @@ | |||
| (ns quo.components.refreshable-scroll-view.view | |||
There was a problem hiding this comment.
Quo components should really be only things related to the design system file.
Imo this seems better to have in status-im.common... ns
I have converted it (not pushed yet), and the result will be like this. Is this acceptable? Screen_Recording_2024-05-03_at_17.12.16.mov |
Looks good @mmilad75! I think we can move the bar at the top that has the total balance out of the list header and it would be good. Wdyt @J-Son89 ? Also, I think it was said that we will remove the graph @J-Son89 ? |
|
Hi @mmilad75 , nice job, thank you! ISSUE 1: Assets order changes every time when you refresh the pageIMG_4690.MP4 |
The order is not handled on the front end side. I don't think there is anything we can do about it. Backend side should fix it. @qoqobolo cc @J-Son89 Please correct me if I'm wrong |
We can handle the ordering on the front end if we want to. But is this change of ordering after refresh happening on this branch only? |
Ah you're right! It's also happening on develop when I reopen the app, I just didn't pay attention to it before tbh. |
So according to design, it looks like we should control the order: from larger to smaller amount of tokens, I guess 
Which doesn't happen now 
But this is an issue that should be logged separately (if it hasn't been already), and I'm not sure about the priority. |
|
Thanks @qoqobolo - let's keep this separate! |
|
Everything looks good to me, thanks for your work @mmilad75! |
mmilad75
force-pushed
the
refresh-control-test
branch
from
da1da2f
to
be2d405
Compare
fixes #19820
UPDATED:
Screen.Recording.2024-05-06.at.19.17.59.mov
Android:
** note that the list of tokens have been tripled manually for test purposes.
Screen.Recording.2024-05-07.at.22.33.17.mov