-
Notifications
You must be signed in to change notification settings - Fork 984
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
Create Navigation mechanism for wizard type flows #19059 #19123
Conversation
Jenkins BuildsClick to see older builds (68)
|
@@ -485,3 +485,15 @@ | |||
:type :negative | |||
:text (i18n/label :t/provider-is-down {:chains chain-names}) | |||
:duration 10000}]]])}))) | |||
|
|||
(def send-asset-flow-config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo this should not be in the events file.
I think a better location for this is:
src/status_im/contexts/wallet/send/flow-config.cljs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about a namespace to store different flow configs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, one or the other. my only thing is that the flow-config should live close to where their data model is used given that the skip-step?
functionality is coupled to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -294,3 +289,35 @@ | |||
{:event :wallet/send-transaction | |||
:error error | |||
:params request-params}))}]}))) | |||
|
|||
(rf/reg-event-fx | |||
:navigation/wizard-send-flow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo this event name should be qualified with :wallet/ as it is related to the wallet.
I think the naming is unclear to what action is happening here too.
I think :wallet/navigate-to-next-step or something would be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure this is necessary at all 🤔
If we replace the :navigate-to-within-stack and :open-modal calls with :navigation/wizard calls (or whatever we rename that event to) this should not be necessary IMO
:flow-config wallet-events/send-asset-flow-config}]))) | ||
|
||
(rf/reg-event-fx | ||
:navigation/wizard-back-send-flow |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we not have a generic wizard event for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
especially this part
(if (= (count stack) 1)
(rf/dispatch [:navigate-back])
(rf/dispatch [:navigate-back-within-stack current-screen])
which is not wallet specific
{:screen-id :wallet-send-input-amount | ||
:skip-step? (fn [db] (some? (get-in db [:wallet :ui :send :amount]))) | ||
:event :wallet/send-select-amount} | ||
{:screen-id :wallet-transaction-confirmation}]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe transaction-progress
is missing from the flow
on-change-tab #(rf/dispatch [:wallet/select-address-tab %]) | ||
input-value (reagent/atom "") | ||
input-focused? (reagent/atom false)] | ||
(fn [] | ||
(let [selected-tab (or (rf/sub [:wallet/send-tab]) (:id (first tabs-data))) | ||
token (rf/sub [:wallet/wallet-send-token]) | ||
;; token (rf/sub [:wallet/wallet-send-token]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we comment this btw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was not used in the code. I forgot to remove the line
(fn [{:keys [db]}] | ||
(let [stack (:modal-view-ids db) | ||
current-screen (last stack)] | ||
(case current-screen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a bit strange to me, for the navigate forward we are passing the event? but for the navigate back we are putting them all in the navigate backwards.
maybe it's cleaner to just write new re-frame events for each and fire them from the given page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we are over-engineering here?
I'd say wizarding logic should only take care of navigation, not side effects of navigating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree 👌
[:navigate-to-within-stack | ||
(if token? | ||
[:wallet-send-input-amount stack-id] | ||
[:wallet-select-asset stack-id])]]]}))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think that might be easier to do this way, as then we don't have to pass the event to the wizard to call,right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps you could provide a code snippet example of how you see that looking @briansztamfater ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I believe that something like this should work. In that way, we only modify navigation logic, and :navigation/wizard
would decide to which screen to navigate next.
(rf/reg-event-fx
:wallet/select-send-address
(fn [{:keys [db]} [{:keys [address token? recipient]}]]
(let [[prefix to-address] (utils/split-prefix-and-address address)
test-net? (get-in db [:profile/profile :test-networks-enabled?])
goerli-enabled? (get-in db [:profile/profile :is-goerli-enabled?])
@@ -64,24 +65,19 @@
(assoc-in [:wallet :ui :send :to-address] to-address)
(assoc-in [:wallet :ui :send :address-prefix] prefix)
(assoc-in [:wallet :ui :send :selected-networks] selected-networks))
:fx [[:dispatch
[:navigation/wizard
{:current-screen :wallet-select-address
:flow-config send-flow-config}]]]})))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For :wallet/send-select-token
, would be
(rf/reg-event-fx :wallet/send-select-token
(fn [{:keys [db]} [{:keys [token]}]]
{:db (-> db
(update-in [:wallet :ui :send] dissoc :collectible)
(assoc-in [:wallet :ui :send :token] token))
:fx [[:dispatch [:wallet/clean-suggested-routes]]
[:dispatch [:navigation/wizard {:current-screen :wallet-select-asset
:flow-config send-flow-config}]]]}))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, we still need to call the previous event which is :wallet/select-send-address
in the screen (and the same for other screens). Both cases seem ok to me @J-Son89 @briansztamfater
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mmilad75 Sounds good to me, IMO that will make the approach simpler and the code easier to understand
src/status_im/navigation/events.cljs
Outdated
(not (when (not (nil? skip-step)) (skip-step db)))))) | ||
flow-config))) | ||
|
||
(rf/reg-event-fx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo it would be nice to have three separate events here.
:navigation/wizard-start
:navigation/wizard-next-step
:navigation/wizard-previous-step
as it's quite obvious from the event of what is happening then.
the lifecycle events of each step can be then easily passed as params if needs be:
e.g on-start
, on-next
, on-previous
, there can also be an on-finish if we need that, but for now it seems not needed 🤔
src/status_im/navigation/events.cljs
Outdated
(when (some? event) | ||
(rf/dispatch [event params])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on why we need this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On each screen, when the submit button is pressed, we're dispatching an event. Now, the event is saved in the config file, and the params are in the screen passed to the navigator. Here, I check, if there is an event, then dispatch it.
(rf/dispatch [:wallet/clean-scanned-address]) | ||
(rf/dispatch [:wallet/clean-local-suggestions]) | ||
(rf/dispatch [:wallet/clean-selected-token]) | ||
(rf/dispatch [:wallet/clean-selected-collectible]) | ||
(rf/dispatch [:wallet/clean-send-address]) | ||
(rf/dispatch [:wallet/select-address-tab nil]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, this logic should remain the same (I am not saying this could not be improved, but at least not be modified in this PR which should only modify navigation stuff).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic is still the same, I just moved them to the navigator 🤔
current-screen (last stack)] | ||
(case current-screen | ||
:wallet-select-address (do | ||
(rf/dispatch [:wallet/clean-scanned-address]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @mmilad75, this should never be done with re-frame. Events are pure functions. dispatch
is a side-effect. Please, use :fx
and core Clojure functions to build the map of effects.
src/status_im/navigation/events.cljs
Outdated
flow-config))) | ||
|
||
(rf/reg-event-fx | ||
:navigation/wizard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a longer comment here could help. How does one use this event properly? What's a flow and how does one configure it?
src/status_im/navigation/events.cljs
Outdated
(when (some? event) | ||
(rf/dispatch [event params])) | ||
(if is-first? | ||
(rf/dispatch [:open-modal (:screen-id next-screen)]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in another comment, re-frame event handlers should return a map with :db
and/or :fx
(the map of effects) describing what needs to be done, but not really do (mutate) anything. The execution should be left to re-frame.
src/status_im/navigation/events.cljs
Outdated
|
||
(rf/reg-event-fx | ||
:navigation/wizard | ||
(fn [{:keys [db]} [{:keys [current-screen flow-config params is-first?]}]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is-first?
-> first?
src/status_im/navigation/events.cljs
Outdated
(rf/reg-event-fx | ||
:navigation/wizard | ||
(fn [{:keys [db]} [{:keys [current-screen flow-config params is-first?]}]] | ||
(let [next-screen (navigate-wizard-next-screen db flow-config current-screen) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name navigate-wizard-next-screen
gives me the impression it will actually navigate, but the function only computes the next screen. I would rename that to something like wizard-find-next-screen
or find-wizard-next-screen
(some people prefer to have the noun before the verb because it helps categorize functions).
(fn [_ [{:keys [current-screen params is-first?]}]] | ||
(when is-first? | ||
(rf/dispatch [:wallet/clean-send-data])) | ||
(rf/dispatch [:navigation/wizard |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as another comment I made: event handlers should compute and have no side-effects.
@mmilad75 I'm missing an explanation both in the PR description, as well as in code about how the wizard mechanism should be used and some decisions that were made to devise it. While reviewing it, I didn't fully understood how should I approach the problem and what to review specifically. For instance, do we want to create a more general wizard like implementation to be used outside the wallet? There are many ways we could go about it. I see the solution is currently only used for the wallet context, so maybe the solution should first grow there, and only once proven and stabilized moved to the general navigation namespace. Was this idea developed in conjunction with other wallet team members? I ask that because I might be missing the context, but anyway the PR and could give more details. |
a9047a7
to
b3a554b
Compare
@@ -8,11 +8,11 @@ | |||
(defn- view-internal | |||
[] | |||
[input-amount/view | |||
{:current-screen-id :screen/wallet.send-input-amount | |||
{:current-screen-id :wallet-send-input-amount |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please revert this 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice Catch 👍
src/status_im/navigation/events.cljs
Outdated
@@ -188,3 +188,30 @@ | |||
(let [view-id (if (= view-id :shell-stack) (shell.utils/calculate-view-id) view-id)] | |||
{:db (assoc db :view-id view-id) | |||
:set-view-id-fx view-id})) | |||
|
|||
(defn wizard-find-next-screen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we mark this as private?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -157,7 +155,6 @@ | |||
:on-press #(rf/dispatch | |||
[:wallet/select-send-address | |||
{:address @input-value | |||
:token? (some? token) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! 🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work @mmilad75, navigation looks much cleaner and easier to extend now :D
src/status_im/navigation/events.cljs
Outdated
(let [stack (:modal-view-ids db) | ||
current-screen (last stack)] | ||
{:fx [[:dispatch | ||
(if (= (count stack) 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder if we can check if current screen is the first in the flow without using modal-view-ids
. After merging #19163 I think we can use :view-id
and compare it with the first screen in the flow that is passed in :navigation/wizard-forward
when start-flow?
is true
(we may need a new key like :wizard/initial-screen
to store it). If that works we could think of removing :modal-view-ids
cc @smohamedjavid @Parveshdhull
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @briansztamfater for pinging me
We should get rid of :navigate-back-within-stack
and just use :navigate-back
. And navigate-back should handle this logic of popping the screen based on where we are in navigation. (within a stack or pushing in root)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if we directly want to pop to a specific screen, we can just use. :navigate-back-to
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, in short we don't need :navigation/wizard-backward
, this should be handled by navigation itself. (what to pop, pushed screen or root itself).
navigate-within-stack is new functionality we introduced for on-boarding. Its still a little immature, and we need to improve it. We should modify navigation code itself according to this feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So before merging this PR
- We merge @briansztamfater's PR - fix: view-id not updating when navigating inside modals #19163
- Then remove modal-view-ids
- Then remove
:navigate-back-within-stack
and add this feature to:navigate-back
itself. - Then if we agree on wizard navigation, then refactor this PR according to new/simple state of navigation.
(Because if we keep implementing new functionality on already complex code, it will harder to simplify it later)
wdyt @briansztamfater @J-Son89 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @J-Son89,
I think Brian's PR is ready to merge, as soon as e2e tests are complete.
I will create issues for task 2 & 3, will complete by Monday.
(feel free to pick these tasks if anyone interested)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: #19254
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Parveshdhull I agree with the approach of getting rid of :navigate-back-within-stack
and handle both cases in :navigate-back
. Currently :navigate-back
always dismisses the current modal, so we should refactor that logic if we want it to support navigating back within modals in that event. Maybe we should remove this line and if we want to dismiss a modal, explicitly dispatch :dismiss-modal
event, or keep that call only if :view-id
it is the first screen in the stack. If the last option is not possible, maybe we would still need :navigation/wizard-backward
which should decide wether to dispatch :navigate-back
or :dismiss-modal
. If we can handle that within :navigate-back
in a generic way, we could definitely remove :navigation/wizard-backward
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completed 2&3, so this PR can be unblocked - #19274
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @mmilad75!
src/status_im/navigation/events.cljs
Outdated
:navigation/wizard-forward | ||
(fn [{:keys [db]} [{:keys [current-screen flow-config start-flow?]}]] | ||
(let [next-screen (wizard-find-next-screen db flow-config current-screen)] | ||
{:fx [[:dispatch | ||
(if start-flow? | ||
[:open-modal (:screen-id next-screen)] | ||
[:navigate-to-within-stack [(:screen-id next-screen) current-screen]])]]}))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be part of navigation or some other namespace wizard/utils
etc. It directly don't look navigation but a specific logic 🤔. @J-Son89 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure it's not core navigation. If you want to move it out of navigation then,
status-im/common/wizard seems like an appropriate place 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure it's not core navigation. If you want to move it out of navigation then, status-im/common/wizard seems like an appropriate place 👍
cc @mmilad75
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Everything should be working fine now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @mmilad75, Thank you
(let [flow-config (case flow-id | ||
:wallet-flow wallet-flow/steps | ||
nil) | ||
next-screen (wizard-find-next-screen db flow-config current-screen)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should just pass flow-id to wizard-find-next-screen
and have this let block there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
79% of end-end tests have passed
Failed tests (9)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (38)Click to expandClass TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
Hi @mmilad75 ! |
Hi @mmilad75 ! Navigation really looks very smooth except one place. ISSUE 1: Select Asset screen is present when we go back in a "Long Press on Asset" scenario
Steps:
Actual result: User sees Select Asset screen that he shouldn't see as he already chosen one at the start IMG_7602.MP4Expected result: Select Asset screen shouldn't be present in this scenario |
33% of end-end tests have passed
Failed tests (6)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Passed tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
Hi @mmilad75 ! Navigation works correct as you've described. |
fixes #19059
fixes #19225
In the wallet designs there are multiple flows which have optional steps depending on the entry point to the flow.
These steps being optional are depending on whether or not the value has been collected or not.
In order to achieve a wizard navigation, a based event called
navigation/wizard-forward
is created. This event receives three paramscurrent-screen
,start-flow?
, andflow-config
.current-screen
is the name of the current screen that is dispatching the event.start-flow?
is a boolean and determines if the navigation flow starts from the current screen that is dispatchingnavigation/wizard-forward
.flow-config
is a vector of maps containing two keysscreen-id
andskip-step?
.screen-id
is the name of the screen which the screen is being registered with.skip-step?
is a function returning a boolean. This determines if the current screen should be skipped or not.There is also another event called
navigation/wizard-backward
which navigates back through the screens passed to thenavigation/wizard-forward
inflow-config
.In order to test the functionality, the QA team should go through sending an asset from different screens such as wallet account home, or long pressing an asset and then sending it.
Here is the flow to follow
Send
button, the following screens should be displayed in order:Send
in the modal, the following screens should be displayed in order:Keep in mind that the navigating back should also be in exact order.