-
Notifications
You must be signed in to change notification settings - Fork 136
[POS as a Tab] Analytics #14225
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
[POS as a Tab] Analytics #14225
Conversation
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
| ORDERS -> AnalyticsEvent.MAIN_TAB_ORDERS_RESELECTED | ||
| PRODUCTS -> AnalyticsEvent.MAIN_TAB_PRODUCTS_RESELECTED | ||
| MORE -> AnalyticsEvent.MAIN_TAB_HUB_MENU_RESELECTED | ||
| else -> null |
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 POS tab cannot be reselected.
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'd consider explicitly listing POS instead of relying on else.
If someone introduces a new BottomNavigationPosition, they might forget to update this when, however, if the when doesn't have else statement, the compiler will inform them about their mistake.
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.
Good point! Done in 10aff47
|
|
||
| override fun onNavigationItemSelected(item: MenuItem): Boolean { | ||
| navController?.let { navController -> | ||
| val navSuccess = NavigationUI.onNavDestinationSelected(item, navController) |
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 don't think it's necessary to call listener.onNavItemSelected only when onNavDestinationSelected is successful. The listener's name suggests that it's triggered when an item is selected, regardless of whether navigation occurred. We need to adjust this for the POS tab, where navigation follows a custom flow and doesn't succeed through the usual mechanism.
…-572-pos-as-a-tab-analytics
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #14225 +/- ##
=========================================
Coverage 37.75% 37.76%
- Complexity 8954 8955 +1
=========================================
Files 1962 1962
Lines 109594 109594
Branches 14366 14364 -2
=========================================
+ Hits 41382 41384 +2
+ Misses 64455 64454 -1
+ Partials 3757 3756 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
malinajirka
left a comment
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.
Thanks @toupper! The changes look good to me.
I've noticed the event is tracked even on phones (with is_visible=false). I assume that is expected and desired, but just noting it to be sure.
🚢
| ORDERS -> AnalyticsEvent.MAIN_TAB_ORDERS_RESELECTED | ||
| PRODUCTS -> AnalyticsEvent.MAIN_TAB_PRODUCTS_RESELECTED | ||
| MORE -> AnalyticsEvent.MAIN_TAB_HUB_MENU_RESELECTED | ||
| else -> null |
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'd consider explicitly listing POS instead of relying on else.
If someone introduces a new BottomNavigationPosition, they might forget to update this when, however, if the when doesn't have else statement, the compiler will inform them about their mistake.
Suspect IssuesThis pull request was deployed and Sentry observed the following issues:
Did you find this useful? React with a 👍 or 👎 |
Description
With this PR we add analytics support for the first iteration of the POS as a Tab project, as well as fix the callback logic for the tab bar that was broken through #14183
Broken callback logic
With
WooPosTabController, we had overridden the entire BottomNavigation listener, which caused some methods in MainActivity.kt not to be called as expected. To fix this, I removed the listener override fromWooPosTabControllerand now invoke that logic directly fromMainActivity.kt's onNavItemSelected.To support this change, I also made a slight adjustment to the logic in
MainBottomNavigationViewto handle the POS mode activation, which doesn't follow the same pattern as the other tabs. See the in-code comment for details.Since this logic was tightly coupled with analytics, I decided to include the fix in the analytics PR—otherwise, analytics wouldn't function correctly. I hope that's okay.
Steps to reproduce
Prerequisite: a WPCOM account that has at least two connected stores, one eligible for POS and the other one not eligible.
🔵 Tracked: woocommerceandroid_pos_tab_visibility_checked, Properties: {"is_visible":"true"...]🔵 Tracked: woocommerceandroid_main_tab_pos_selected.🔵 Tracked pos_tab_visibility_checked, properties: [is_visible: false, ...]Testing information
I also verified that the rest of the main bottom navigation logic continues to work as expected.
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.