chore: refactor screens definitions and add more navigation events for screens #21328
Conversation
Jenkins BuildsClick to see older builds (73)
|
|
Hi @seanstrom My thoughts on this PR: I don't clearly see the problem we want to solve with this PR. To me, some categories are not clearly defined: e.g.
Since I don't see a clear definition of the categories, I think the current PR provides the same clarity as what we had achieved only with comments before (i.e. On the other hand, I think it'd be great that our screens were grouped, ideally (to me), our file structure and our screen groups should reflect in some way the navigation structure, so that adding/organizing/finding a screen is easy.
Is it possible for you to share the plans on how to achieve it? Sorry if I did a lot of questions, It's just a bit unclear to me the benefits from this PR. Thanks @seanstrom ! |
|
Hey @ulisesmac, these are good questions, thanks for the review 🙏 This PR is attempting to group the different screens into separate functions, but as you have mentioned they're not a precise enough abstraction over all of the screens. That being said the main benefit for grouping them into these "zones" will help with detecting which "kind" of screens is being displayed when we're tracking the navigation events. For example, if we want to track more screen navigation events, but we want to be careful about which screens we're tracking, then we can reference a group of screens directly and extract the names of the screens. Here's some example code of what I'm thinking would be done in practice: ;; screen definitions
(defn contact-screens
[]
[{:name :new-contact
:options {:sheet? true}
:component add-new-contact/new-contact}
{:name :scan-profile-qr-code
:options options/dark-screen
:component scan-profile-qr-page/view}
{:name :contact-profile
:options {:modalPresentationStyle :overCurrentContext}
:component contact-profile/view}
{:name :share-contact
:options options/transparent-screen-options
:component share-contact/view}])
(defn onboarding-screens
[]
[{:name :screen/onboarding.intro
:options {:theme :dark}
:on-focus [:onboarding/overlay-dismiss]
:component intro/view}
{:name :screen/onboarding.syncing-results
:options {:theme :dark}
:component syncing-results/view}])
;; tracking
(def ^:const view-ids-to-track
(-> (concat (onboarding-screens) (contact-screens))
(map :name)
(into #{;; Tabs
:communities-stack
:chats-stack
:wallet-stack})))
(defn track-view-id-event
[view-id]
(when (contains? view-ids-to-track view-id)
(navigation-event (name view-id))))
(defn tracked-event
[[event-name second-parameter]]
(case event-name
:profile/get-profiles-overview-success
(user-journey-event app-started-event)
:centralized-metrics/toggle-centralized-metrics
(key-value-event "events.metrics-enabled" :enabled second-parameter)
:set-view-id
(track-view-id-event second-parameter)
nil))
In this example we can see that the tracking code is now referencing the screen names without duplicating the names from the screens file, and it also helps us focus a specific subset of screens. And if we can focus on a subset then we can add additional metadata or logic for processing those navigation events, or even limit the amount of metadata when we think the event could represent sensitive information (like a wallet transaction or something).
Yeah this would be an ideal for me too, though I haven't attempted to re-structure things atm. For now I'm relying on the screens file to be source-of-truth for all the screen names.
Yeah we can continue to improve this PR to have better groups or aliases for the screens, atm I decided to try something minimal with low-risk of breaking things, but we can continue to refine this approach in this PR or as we go. Though I did plan to experiment with the tracking code in a separate PR when we start expanding the navigation events, but I suppose we can at least replace the onboarding detection and possibly add more screens to the tracking logic too. I hope this commentary helps clarify things, and feel free to share more too. I imagine that this will be a collective effort for managing screens over time, so we should have something that we can all maintain and enjoy 🙌 |
|
@seanstrom: Grouping screens in different vars is a nice improvement, I see no harm in that, but using that as a proxy for meaning trackable I think is mixing orthogonal concerns. It will cause trouble later because we will need to regroup screens to indirectly track/not track them. A more direct solution, which also doesn't rely on hierarchies is: we can add a flag to each navigation screen map in case it is allowed to be tracked. Then, in the tracking namespace, similar to what you shared above, we statically build a set of The code would be trivial: {:name :screen/xyz
:track? true ; => ********* ADDED
:options {:insets {:top? true}}
:component ...}
;; In the ns status-im.contexts.centralized-metrics.tracking
(def ^:const view-ids-to-track
(into #{;; Tabs
:communities-stack
:chats-stack
:wallet-stack}
(->> (screens/screens)
(filter :track?)
(map :name))))About the discussion of code organization of the screens, I kind of prefer a central place than spreading them over directories, which would make finding them even more difficult, just 2 cents. One thing that's been on my radar for a long time was to extract each screen map into a separate var because that would instantly give us reference finding, unused screens, and jumping to source. Would be super helpful to everybody if we did this 🙊 |
There was a problem hiding this comment.
@ilmotta @seanstrom
After considering your points, I'm approving this PR now since I agree on the benefit of tracking data.
And, just to clarify, ideally, I'd like to have a Clojure data structure and a file hierarchy that refelects the navigation:
e.g. for files
root
|
|--- Onboarding
|
|--- Welcome
|--- Sign-in
|
|--- Wallet
|
|-- Home
|
|--- Communities
|--- Messsages
|--- Browser
Something similar to [Expo Router (used for React Native)](Introduction to Expo Router)
It brings the best file-system routing concepts from the web to a universal application — allowing your routing to work across every platform. When a file is added to the app directory, the file automatically becomes a route in your navigation.
So it'd impact our (defn screens [] ...), we could even create a macro to do the job, but we first need to make clear what is the best data structure.
seanstrom
force-pushed
the
seanstrom/chore-refactor-screens-definitions
branch
from
bfc0646
to
e74212c
Compare
|
@ilmotta @ulisesmac thanks for the feedback 🙏 I agree that we should add more data to each screen for configuring |
seanstrom
force-pushed
the
seanstrom/chore-refactor-screens-definitions
branch
from
2357c20
to
23aded7
Compare
|
Okay I've made some changes based on some experiments and your feedback. Now we can configure if a screen is tracked by tagging the screen definition with some extra metadata, and we can also alias the event-id / event name that gets sent to Mixpanel too. We also have the ability to send screen specific events (if we want to), please let me know what you think when you have a chance 🙏 |
@ulisesmac This reminds of the way Rails made convention over configuration famous more than 15 years ago 🚋 I tend to prefer more explicit configuration, rather than implicit because it's simpler to accommodate for pretty much any use case. But I don't have any strong preference, but other CCs may have. |
There was a problem hiding this comment.
I'm leaving a few minor suggestions @seanstrom. As usual, great work with the thoughtful implementation.
status-mobile.code-workspace
Outdated
| @@ -0,0 +1,10 @@ | |||
| { | |||
| "folders": [ | |||
There was a problem hiding this comment.
Just checking, is this file being intentionally added? Good for other devs too?
There was a problem hiding this comment.
ooops, this is my special file 🙈
| @@ -17,7 +17,10 @@ | |||
| (when-let [event (tracking/tracked-event (interceptor/get-coeffect context :event))] | |||
| (log/debug "tracking event" event) | |||
| (when (push-event? (interceptor/get-coeffect context :db)) | |||
| (native-module/add-centralized-metric event))) | |||
| (cond | |||
| (and (coll? event) | |||
There was a problem hiding this comment.
@seanstrom, coll? will return true if event is a map, for example. Maybe here we should only use vector? to be extra precise?
Could be an if instead of cond. doseq is preferred to process with side-effects and ignore results.
(if (vector? event)
(doseq [e event]
(native-module/add-centralized-metric e))
(native-module/add-centralized-metric event))There was a problem hiding this comment.
Ah good call for using doseq 👍
I think you're right that we can remove the coll? check. Additionally, I think we can also support seqs by using seq? since we might return a sequence or vector from the tracking function, how does that sound?
| (defn community-screens | ||
| [] | ||
| [{:name :discover-communities | ||
| :metrics {:flow :community |
There was a problem hiding this comment.
I'm on the fence about adding :flow now because we aren't using this data yet, or maybe I missed in the diff. Given that event.id keyword already has a useful qualification, in the analytics tool this may be sufficient for filtering purposes (not sure).
There was a problem hiding this comment.
Happy to remove these values 👍
For context: currently :flow is not used for anything, so we do not need that property, it's mostly there as a way to categorise the screens without relying too much on the variable names in the file.
There was a problem hiding this comment.
+1 for removing them, since we are already categorizing them with vars
There was a problem hiding this comment.
I also feel we should not add it here. Flow can be inferred from the data.
status-mobile.code-workspace
Outdated
| @@ -0,0 +1,10 @@ | |||
| { | |||
| "folders": [ | |||
There was a problem hiding this comment.
ooops, this is my special file 🙈
seanstrom
force-pushed
the
seanstrom/chore-refactor-screens-definitions
branch
2 times, most recently
from
e116aaf
to
50d6a32
Compare
seanstrom
force-pushed
the
seanstrom/chore-refactor-screens-definitions
branch
from
7e1b94d
to
2cf7918
Compare
seanstrom
changed the title
seanstrom
force-pushed
the
seanstrom/chore-refactor-screens-definitions
branch
from
2cf7918
to
a0ac154
Compare
| (defn community-screens | ||
| [] | ||
| [{:name :discover-communities | ||
| :metrics {:flow :community |
There was a problem hiding this comment.
+1 for removing them, since we are already categorizing them with vars
…ridge navigation events
… sharing of metrics
seanstrom
force-pushed
the
seanstrom/chore-refactor-screens-definitions
branch
from
e5a11de
to
3c9fdb0
Compare
|
Hey @seanstrom ! Is PR ready to be tested? Or you plan to do more changes? |
Summary
Updated Summary
Testing notes
Platforms
Areas that maybe impacted
Functional
Steps to test
Before and after screenshots comparison
WIP
status: ready
Risk
Described potential risks and worst case scenarios.
Tick one: