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.
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.
How about a namespace to store different flow configs?
There was a problem hiding this comment.
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.
| @@ -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.
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.
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.
should we not have a generic wizard event for this?
There was a problem hiding this comment.
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.
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.
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.
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.
Maybe we are over-engineering here?
I'd say wizarding logic should only take care of navigation, not side effects of navigating.
| [:navigate-to-within-stack | ||
| (if token? | ||
| [:wallet-send-input-amount stack-id] | ||
| [:wallet-select-asset stack-id])]]]}))) |
There was a problem hiding this comment.
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.
perhaps you could provide a code snippet example of how you see that looking @briansztamfater ?
There was a problem hiding this comment.
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.
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.
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.
@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.
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.
Can you elaborate on why we need this?
There was a problem hiding this comment.
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.
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.
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.
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.
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.
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?]}]] |
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.
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.
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. |
mmilad75
force-pushed
the
milad/19059-create-new-navigation-flow
branch
from
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 | |||
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 | |||
| @@ -157,7 +155,6 @@ | |||
| :on-press #(rf/dispatch | |||
| [:wallet/select-send-address | |||
| {:address @input-value | |||
| :token? (some? token) | |||
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.
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.
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.
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.
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.
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-stackand add this feature to:navigate-backitself. - 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.
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.
@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.
Completed 2&3, so this PR can be unblocked - #19274
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.
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.
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.
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
Everything should be working fine now |
| (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.
Maybe we should just pass flow-id to wizard-find-next-screen and have this let block there.
79% of end-end tests have passedFailed 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 passedFailed 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-forwardis created. This event receives three paramscurrent-screen,start-flow?, andflow-config.current-screenis 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-configis a vector of maps containing two keysscreen-idandskip-step?.screen-idis 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-backwardwhich navigates back through the screens passed to thenavigation/wizard-forwardinflow-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
Sendbutton, the following screens should be displayed in order:Sendin the modal, the following screens should be displayed in order:Keep in mind that the navigating back should also be in exact order.