Skip to content
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 track events #108

Merged
merged 9 commits into from
Jun 2, 2023
Merged

Add track events #108

merged 9 commits into from
Jun 2, 2023

Conversation

karenroldan
Copy link
Contributor

What Does This PR Add/Change?

Add track events based on pciE2j-27d-p2

  1. issue_search
  2. status_filter_select
  3. repo_filter_select
  4. sort_select
  5. issue_link_click
  6. navbar_report_issue_start
  7. banner_report_issue_start
  8. navbar_item_click

Testing Instructions

  • Tests are passing
  • Make sure the events are recorded as expected. You can also find it in the console - [Event recorded]:

Issues

Related to #
Closes #

@karenroldan karenroldan requested a review from a team May 25, 2023 15:57
@karenroldan karenroldan mentioned this pull request May 25, 2023
2 tasks
Copy link
Contributor

@john-legg john-legg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, Karen! I had a few compile errors that I pointed out below. I fixed them locally, so I could continue testing your PR, but please take a look when you have a chance!

Once I got Bugomattic working, I checked all of the events and they fired as expected. I just had one comment about naming, but that's it.

dispatch( setActivePage( page ) );
dispatch( updateHistoryWithState() );

monitoringClient.analytics.recordEvent( 'navbar_item_click', { page } );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue (blocking):
I'm not sure if you're seeing it on your end, but I actually had a couple of compile errors. One was on line 92:

TS2304: Cannot find name 'monitoringClient'.
    90 | 		dispatch( updateHistoryWithState() );  
    91 |
  > 92 | 		monitoringClient.analytics.recordEvent( 'navbar_item_click', { page } );
       | 		^^^^^^^^^^^^^^^^
    93 | 	};
    94 |  
    95 | 		return (

I'm not sure if something happened the recent force push, but I just added const monitoringClient = useMonitoring(); under SimpleMenuItem to get it working.

I also got this error on line 100:

TS2304: Cannot find name 'handleClick'.
     98 | 				role="menuitem"   99 | 				aria-current={ currentActivePage === page ? 'page' : undefined }
  > 100 | 				onClick={ handleClick( page ) }
        | 				          ^^^^^^^^^^^  101 | 				tabIndex={ tabIndex }
    102 | 				className={ styles.menuItem }
    103 | 			>

I just changed it to handleMenuItemClick to get it working.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a few compile errors that I pointed out below

AAAAAH. My bad! I had two VS Codes (one is the Insiders one since I'm playing with the Copilot chat) open and didn't see the error after "fixing" the merge conflicts! It should be fixed now.

Thank you for the heads up! 🙇🏻‍♀️

await user.click( screen.getByRole( 'option', { name: 'Date added' } ) );

expect( monitoringClient.analytics.recordEvent ).toHaveBeenCalledWith( 'sort_select', {
sortOption: 'date-created',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick (non-blocking):
I noticed that the event name is date-created, whereas the filter in the UI is Date added. This made me realize that we've been a bit inconsistent with our naming of this field because even the duplicate search result row uses Opened 4 days ago lol.

I feel like we should probably pick one and keep it the same throughout duplicate search 😆 For example, the filter and event name could be date created and the results row could display Created 4 days ago.

Karen and @dpasque, what are your thoughts on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should probably pick one and keep it the same throughout duplicate search 😆 For example, the filter and event name could be date created and the results row could display Created 4 days ago.

That's a good point! Since date created is used mostly everywhere, we can update the text in the dropdown from Date added to Date created.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created a separate PR for this change: #110

@john-legg
Copy link
Contributor

This looks great, Karen! The code compiled properly and all of the track events work as expected 😄

suggestion (non-blocking):
I did want to follow up on one note I made in the previous comment

This made me realize that we've been a bit inconsistent with our naming of this field because even the duplicate search result row uses Opened 4 days ago lol.

For the duplicate search result rows, would it be possible to change the word Opened to Created to better match the renamed Date Created sorting option? This isn't urgent, so it can be in a separate PR if the word change sounds reasonable to you!

@karenroldan
Copy link
Contributor Author

For the duplicate search result rows, would it be possible to change the word Opened to Created to better match the renamed Date Created sorting option?

Ofc, I missed the previous text! 🤦🏻‍♀️ it should be fixed in #113. Thanks again for checking.

@karenroldan karenroldan merged commit a185823 into trunk Jun 2, 2023
@karenroldan karenroldan deleted the add/track-events branch June 2, 2023 07:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants