-
Notifications
You must be signed in to change notification settings - Fork 985
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
chore: refactor screens definitions and add more navigation events for screens #21328
base: develop
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
bfc0646
to
e74212c
Compare
@ilmotta @ulisesmac thanks for the feedback 🙏 I agree that we should add more data to each screen for configuring |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just checking, is this file being intentionally added? Good for other devs too?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for removing them, since we are already categorizing them with vars
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 also feel we should not add it here. Flow can be inferred
from the data.
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.
Self-Review 📓
status-mobile.code-workspace
Outdated
@@ -0,0 +1,10 @@ | |||
{ | |||
"folders": [ |
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.
ooops, this is my special file 🙈
e116aaf
to
50d6a32
Compare
7e1b94d
to
2cf7918
Compare
2cf7918
to
a0ac154
Compare
(defn community-screens | ||
[] | ||
[{:name :discover-communities | ||
:metrics {:flow :community |
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.
+1 for removing them, since we are already categorizing them with vars
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.
Looks good!
…ridge navigation events
… sharing of metrics
e5a11de
to
3c9fdb0
Compare
Hey @seanstrom ! Is PR ready to be tested? Or you plan to do more changes? |
Hey @mariia-skrypnyk i think the PR is ready, but there are some other PRs that will cause me to do a rebase, so we may want to wait for those to merge first. Specifically PR #21379 is scheduled to merge before this PR |
@seanstrom thanks, so should I start testing after merg of #21379 ? |
@mariia-skrypnyk yup yup, after it merges and I rebase to include the changes in this PR |
Please, tag me when it will be ready for me 🙏 |
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: