-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Adds more tracking to the reader, comments, and notifications #17558
Conversation
You can trigger an installable build for these changes by visiting CircleCI here. |
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
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.
Tap the 'Turn on site notifications' button
It took me a while to find a post that had this option, and I'm not sure why!
PR looks good! Just one small code comment!
private enum SearchSource { | ||
case userInput | ||
case searchHistory | ||
|
||
var value: String { | ||
switch self { | ||
case .userInput: return "user_input" | ||
case .searchHistory: return "search_history" | ||
} | ||
} | ||
} |
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.
Could this be simplified a little to:
private enum SearchSource: String {
case userInput = "user_input"
case searchHistory = "search_history"
}
and then use rawValue
where we were using value
?
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.
That's a way better idea than what I had. Updated the code: 035d8dc
@emilylaguna I'm going to bump this to the next release because we'll be code freezing 18.8 today and this hasn't been approved yet. @frosty basically said this is good to go, but there are also conflicts against If this cannot wait two weeks and it's important that it makes it into this release, let me know and we'll organize a new beta once ready. |
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.
Project: #17503
Description
Adds new tracking to the Reader, Comments, and Notifications:
Reader
Comments
Notifications
To test:
Reader Search
🔵 Tracked: reader_search_performed <source: user_input, type: posts>
🔵 Tracked: reader_search_performed <source: user_input, type: sites>
🔵 Tracked: reader_search_performed <source: search_history, type: posts>
🔵 Tracked: reader_search_history_cleared <subscription_count: COUNT>
Reader Toggle Follow
🔵 Tracked: followed_blog_notifications_reader_menu_on <blogId: BLOG_ID, subscription_count: COUNT>
🔵 Tracked: followed_blog_notifications_reader_menu_off <blogId: BLOG_ID, subscription_count: COUNT>
Comments
🔵 Tracked: comment_fullscreen_entered <>
🔵 Tracked: comment_fullscreen_exited <>
Notifications
🔵 Tracked: notifications_previous_tapped <>
🔵 Tracked: notifications_next_tapped <>
To me it feels a little weird that the down arrow is previous and up is next, this matches the naming in code.
Regression Notes
Potential unintended areas of impact
None, adding tracking.
What I did to test those areas of impact (or what existing automated tests I relied on)
Manual testing
What automated tests I added (or what prevented me from doing so)
None.
PR submission checklist:
RELEASE-NOTES.txt
if necessary.