[#18495] Fix token not ready for next screen #18532
Conversation
ulisesmac
requested review from
ilmotta,
briansztamfater,
J-Son89 and
OmarBasem
Jenkins BuildsClick to see older builds (53)
|
ulisesmac
force-pushed
the
18495-wrong-asset-selection
branch
from
af408c7
to
aac298c
Compare
|
Update, since the change to solve: Was very small, I decided to add it in this PR |
ulisesmac
force-pushed
the
18495-wrong-asset-selection
branch
from
a2800d7
to
94f9843
Compare
|
QA notes: Check issue is not reproducible in this PR. |
|
@ulisesmac thanks for the PR. Please, do not forget to move PRs from REVIEW to E2E column once PR is reviewed and ready for QA. Could you please rebase current PR, resolve conflicts and after that move to E2E column to trigger e2e run? Thank you. |
ulisesmac
force-pushed
the
18495-wrong-asset-selection
branch
2 times, most recently
from
da08559
to
1e516b4
Compare
Done! thanks |
Hey @ulisesmac, me again :) Something tells me we should just not have these issues if the code assumed that data will eventually arrive and out of order due to the asynchronicity of React Navigation, RPC requests, JS timers here and there, animations and so on.
Hence, each individual effect may cause other operations to be scheduled, which will, in turn be processed in a separate queue or they will dispatch async operations (e.g. Navigation). With this principle in mind, if our views are all properly subscribing to data, it shouldn't matter if the navigation finishes before the state is propagated, because the screen will react accordingly. If we want to avoid the view from flashing on screen with empty elements, then as @Parveshdhull said, we have techniques for loading indicators. So to summarize my long train of thought, I believe by implementing the bare bones good practices to use re-frame (or any Redux app for that matter) we will arrive at something that just works, e.g no subs in the mount phase to initialize local state. |
| :fx [[:dispatch-later | ||
| {:ms 1 | ||
| :dispatch [:navigate-to-within-stack [:wallet-send-input-amount stack-id]]}]]})) | ||
| :fx [[:navigate-to-within-stack [:wallet-send-input-amount stack-id]]]})) |
There was a problem hiding this comment.
Thanks @ulisesmac for warning about this PR and discussion! We need to update DB when navigating, so fx might not be what we want here. This would be replaced with a dispatch if we merge #18593, so better to reach to a common agreement before merging any of these PRs to avoid unnecessarily reverting changes. Context: #18593 (comment)
There was a problem hiding this comment.
Also, another topic, probably for other PR, but it is confusing to have navigation effects and events with same naming, like
and.......
or
....... and
We should rename events to have navigation / namespace prefix as per our guidelines
status-mobile/doc/new-guidelines.md
Lines 364 to 372 in 5cc027a
so events are like :navigation/navigate-back, :navigation/navigate-back-to, :navigation/navigate-to, :navigation/navigate-to-within-stack, etc, or even better they could be improved as :navigation/pop, :navigation/pop-to, :navigation/push, :navigation/push-within-stack, etc, to comply with common nomenclature for stack navigations
|
It seems the approach the team want to take is to make the view reactive to these values. TBH I don't agree, if the
Having a loading indicator for views that expect incoming data is completely OK, but having a loader for local state propagation is too much IMO. In this case the user picks a token to make a transaction, then we navigate and the new view will "load" for ~25 ms, after that we'll show the token the user previously picked. No network calls or storage read involved in this process and we are "loading" or blinking. I think designing/implementing a loader component for these 25 ms is too much. At least for me, prod builds are facing this issue almost all the time. So I'm going to solve it with a temporary blink and I'll open another issue to check what kind of loader we'll show. |
Completely agree here @ulisesmac, for local state sounds like a smell to have a loading indicator. Loading indicators should be used sparingly. Anything under ~100-200ms and it might be better to just not show the information until the data is ready, otherwise the user will see the indicator flash on screen pretty rapidly. The thing to me is to understand what are the minimum data necessary to render the screen. It's about the granularity of the loading strategy. If the data also includes local state, so be it. The point is that the user shouldn't see too many elements flashing on screen (especially big ones) or see temporarily incomplete values while the data hasn't arrived (a problem we currently have when fetching permissions for a community). Spotify on iOS happily shows me big 3 dots when the screen is not cached yet, and pop in small elements that arrive 1-2s later. Our designers don't like this approach, so sometimes it's also about personal preference. Whatever the best UX is, that's where we should go 👍🏼 |
ulisesmac
force-pushed
the
18495-wrong-asset-selection
branch
2 times, most recently
from
6137416
to
04d0c58
Compare
|
Thanks to everyone for the effort reviewing this PR. @VolodLytvynenko This PR is ready to get reviewed again, the issue is solved, however, there's a short blink when we visit the screen, it'll be addressed separately when this PR gets merged |
85% of end-end tests have passedFailed tests (5)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Expected to fail tests (2)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (41)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
Hi @ulisesmac. PR is tested and ready to be merged. Thank you for your work! |
- Makes Token malli schema more lenient - Make `get-standard-crypto-format` able to work with `nil` values - Refactor token screen to move subscriptions inside render function
ulisesmac
force-pushed
the
18495-wrong-asset-selection
branch
from
d015011
to
a6b5616
Compare
* Fix exception thrown re-frame don't have the subs' value ready * Make Token malli schema more lenient * Make `get-standard-crypto-format` able to work with `nil` values * Refactor token screen to move subscriptions inside render function * Make token input reactive to on-swap * Remove `crypto-currency?` atom to properly react to state changes * Fix component tests
fixes #18495
fixes #18524
fixes #18493
Update:
fixes #18469
Summary
This PR fixes the exception thrown whilke picking an asset to send in this screen:
And after that, we needed to pick twice the same token in order to actually send it, now it works fine.
Review notes
The behavior happened because re-frame didn't had the db changes ready before navigating, IDK why this is happening.
Steps to test
The behavior is now fixed.
status: ready