-
Notifications
You must be signed in to change notification settings - Fork 9
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
Add search and sort observability metrics #3007
Add search and sort observability metrics #3007
Conversation
} | ||
} | ||
|
||
class SortBookmarksViewModelTests: XCTestCase { |
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.
Awesome, really glad to see how thoroughly tested this is!
d240c73
to
fc01a2f
Compare
820a594
to
a217b5a
Compare
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 have one comment, but otherwise testing looks good
case manager | ||
} | ||
|
||
struct BookmarksSearchAndSortMetrics { |
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.
What's the motivation behind this versus using PixelKit directly?
In general I'd say we use PixelKit directly, and I'd rather keep things consistent unless there's a good reason for it
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 prefer to encapsulate this kind of logic; if you have multiple fires to the same pixel and need to change something (for example, the frequency), you have only one place to do it.
Finding the pixels related to a specific feature is easier. Now, you need to navigate through the large pixel list in the GeneralPixel
enum.
Do you think it’s a blocker for this PR?
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.
Nah it's not a blocker. I think your reasoning makes sense, but I would rather do it or not do it universally, and think the consistency is more valuable than the benefits of either approach.
aadfa2b
into
feature/search-and-sort-bookmarks-panel
Task/Issue URL: https://app.asana.com/0/1199230911884351/1207704569751027/f
Tech Design URL:
CC:
Description
Adds pixels to observe interactions with search and sort.
Steps to test this PR
Sort button clicked
sort_bookmarks_button_clicked
should be firedSort button dismissed
sort_bookmarks_button_dismissed
should be fired.Sort mode by name
Name
option. Repeat from 2, and selectAscending
andDescending
sort_bookmarks_by_name
should be fired.Search executed
search_bookmarks_executed
should be fired. This pixel is fired daily.Search result clicked
search_result_clicked
should be fired when the bookmark an folder are tapped.Definition of Done
—
Internal references:
Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation