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

Gutenboarding: Add new tracking events #42099

Merged
merged 8 commits into from May 13, 2020

Conversation

razvanpapadopol
Copy link

@razvanpapadopol razvanpapadopol commented May 12, 2020

Changes proposed in this Pull Request

  • Add tracking for vertical input when moving the focus out of the field or selecting a vertical.
    • selected_vertical_slug - The slug of the selected topic, defined only when the vertical is not free-form user input.
    • selected_vertical_label - Translated label of vertical or free-form user input.
  • Add tracking for site title input when moving the focus out of the field.
    • has_selected_site_title - Whether site title has been entered on the acquire intent page.
  • Add tracking for skipped vertical or site title inputs.
  • Update calypso_newsite_step_leave event for IntentGathering step:
    • add selected_vertical_label prop which can be free-form user input.
    • change selected_vertical prop to selected_vertical_slug recording the same value as before
    • change selected_site_title prop to has_selected_site_title recording if a site title has been entered or not.
  • Add tracking to PlansModal (open and closed with selected plan).
  • Add tracking to signup controller to record the calypso_newsite_init before redirecting users to Gutenboarding.

Testing instructions

  • The calypso_newsite_vertical_selected event should be fired when moving the focus off the vertical input that has a value (tabbing, clicking away, pressing enter or selecting a vertical) or when clearing the input.
  • The calypso_newsite_site_title_selected event should be fired when moving the focus off the site title input (tabbing, clicking away).
  • The calypso_newsite_vertical_skipped event should be fired when pressing grey I donʼt know link below it.
  • The calypso_newsite_site_title_skipped event should be fired when pressing blue I donʼt know button or pressing Enter when focused on empty Site Title input to advance to Design Picker step.
  • When advancing to Design Picker step, the calypso_newsite_step_leave event should should fire with props {flow: "gutenboarding", name: "IntentGathering", selected_vertical_slug: string | undefined, selected_vertical_label: string | undefined, has_selected_site_title: boolean}
  • The calypso_newsite_modal_open event should fire with props {flow: "gutenboarding", name: "PlansGrid"} when clicking the Plans Button.
  • The calypso_newsite_modal_close event should fire with props {flow: "gutenboarding", name: "PlansGrid", selected_plan: "free_plan"} when closing the Plans modal having the last selected plan slug instead of "free_plan".
  • The calypso_newsite_init event should fire when A/B test condition is true, before redirecting user to /new.

Related to: #41059

@razvanpapadopol razvanpapadopol added [Feature] Tracks Metrics & Monitoring Capturing analytics about user behavior on WordPress.com. [Goal] New Onboarding previously called Gutenboarding labels May 12, 2020
@razvanpapadopol razvanpapadopol requested a review from a team as a code owner May 12, 2020 14:19
@razvanpapadopol razvanpapadopol self-assigned this May 12, 2020
@matticbot
Copy link
Contributor

@razvanpapadopol razvanpapadopol requested a review from a team May 12, 2020 14:19
@matticbot
Copy link
Contributor

matticbot commented May 12, 2020

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

App Entrypoints (~170 bytes added 📈 [gzipped])

name                 parsed_size           gzip_size
entry-gutenboarding       +570 B  (+0.0%)     +170 B  (+0.0%)

Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used.

Sections (~19 bytes added 📈 [gzipped])

name    parsed_size           gzip_size
signup        +95 B  (+0.0%)      +19 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@razvanpapadopol razvanpapadopol added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label May 12, 2020
Copy link
Member

@alshakero alshakero left a comment

Choose a reason for hiding this comment

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

This created a regression. When you select a plan, it's immediately confirmed without waiting for a click on "Confirm".

I suggest we keep the isOpen prop and use an effect hook that registers an event whenever isOpen changes. This way we avoid stale-state problems.

@razvanpapadopol
Copy link
Author

This created a regression. When you select a plan, it's immediately confirmed without waiting for a click on "Confirm".

It's already like this on master. Confirm and Close are both just closing the modal. The change has been made in #42028 to align behaviour between Plans page and Plans Modal so that when the user press Select on a plan, they immediately see the plan button label update (discussed with @dubielzyk).

I suggest we keep the isOpen prop and use an effect hook that registers an event whenever isOpen changes. This way we avoid stale-state problems.

I was previously going this route and having the modal always mounted but useTrackModal is expecting the component to be unmounted to fire the _close event. We're doing the same with Domain Picker modal and I don't expect to cause any problems.

Copy link
Member

@p-jackson p-jackson left a comment

Choose a reason for hiding this comment

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

Haven't finished testing yet but submitting review before GH looses my words 😅

client/landing/gutenboarding/lib/analytics/index.ts Outdated Show resolved Hide resolved
client/landing/gutenboarding/lib/analytics/index.ts Outdated Show resolved Hide resolved
client/signup/controller.js Outdated Show resolved Hide resolved
client/signup/controller.js Outdated Show resolved Hide resolved
@p-jackson
Copy link
Member

Found a few things during manual testing:

  • Repeatedly focusing/unfocused the text fields without typing anything keeps sending events even when there's no change. Might not matter.
  • When entering a title and pressing "Enter" on the keyboard, the calypso_newsite_title_selected event isn't being sent. Maybe the component unmounts before we get a change to handle the onblur event?
  • If the user tabs out of an empty vertical field (perhaps to press skip), it's not sending calypso_newsite_vertical_selected. Tabbing out of an empty title field does send calypso_newsite_site_title_selected though. If we want to see empty values for calypso_newsite_*_selected events then we should do what the title field does.

@razvanpapadopol
Copy link
Author

razvanpapadopol commented May 13, 2020

Thanks for reviewing this and for the well thought suggestions @p-jackson 💯

  • Repeatedly focusing/unfocused the text fields without typing anything keeps sending events even when there's no change. Might not matter.

To send *_site_title_selected events only when there's a change we need to add some extra state in current implementation. It can be done only if the noise would be a problem in reports.

  • When entering a title and pressing "Enter" on the keyboard, the *_site_title_selected event isn't being sent. Maybe the component unmounts before we get a change to handle the onblur event?

Probably yes. But we are supposed to track the change events before the user advances to the next step (or drops). p1589270549002500-slack-luna
And the new value selected would be send with the *_step_leave event anyway.

  • If the user tabs out of an empty vertical field (perhaps to press skip), it's not sending calypso_newsite_vertical_selected. Tabbing out of an empty title field does send calypso_newsite_site_title_selected though. If we want to see empty values for calypso_newsite_*_selected events then we should do what the title field does.

We're sending calypso_newsite_vertical_selected if the users clears a field which had a value. In that case we're actually saving the new (empty) selection. p1589268265495500-slack-luna

@simison
Copy link
Member

simison commented May 13, 2020

To send *_site_title_selected events only when there's a change we need to add some extra state in current implementation. It can be done only if the noise would be a problem in reports.

Would setting a flag in state that the event has been sent already once work? Without tracking if something has changed in the input, as I don't think that matters. I think in practise we really need the event just once.

@razvanpapadopol
Copy link
Author

razvanpapadopol commented May 13, 2020

To send *_site_title_selected events only when there's a change we need to add some extra state in current implementation. It can be done only if the noise would be a problem in reports.

Would setting a flag in state that the event has been sent already once work? Without tracking if something has changed in the input, as I don't think that matters. I think in practise we really need the event just once.

I was thinking about saving the previous value recorded in state to compare it with before sending a new event.

The requirement was to send an event whenever we store the value, while the user is interacting with this step. Because we're storing the Site Title with every keystroke, I've decided to send an event only when the user is leaving the input. This means they finished typing and are going to do another action.

I don't think it's a bug sending multiple events with the same value. It just means the user is focusing in and out the field without making a change which could raise some UX concerns.

@simison
Copy link
Member

simison commented May 13, 2020

The requirement was to send an event whenever we store the value, while the user is interacting with this step. Because we're storing the Site Title with every keystroke, I've decided to send an event only when the user is leaving the input. This means they finished typing and are going to do another action.

Ah gotcha, yeah that sounds reasonable.

I don't think it's a bug sending multiple events with the same value. It just means the user is focusing in and out the field without making a change which could raise some UX concerns.

Agreed. 👍 Events are cheap. :-)

Copy link
Member

@simison simison left a comment

Choose a reason for hiding this comment

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

Tested that all events fire as described.

Tested opening/closing plans grid as a modal, as well as a step as well.

Razvan Papadopol and others added 8 commits May 13, 2020 14:49
* Update call to IntentGathering useTrackStep to send also the free-form text entered in the vertical input.
…ertical-select/index.tsx

Co-authored-by: Omar Alshaker <omar@omaralshaker.com>
* Remove free_form prop from calypso_newsite_vertical_selected event.
* Use selected_vertical_slug and selected_vertical_label to track vertical selection.
@razvanpapadopol razvanpapadopol force-pushed the add/gutenboarding-new-tracking-events branch from ca2de03 to d6adedc Compare May 13, 2020 11:49
@simison simison added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 13, 2020
@razvanpapadopol razvanpapadopol merged commit 4b0f529 into master May 13, 2020
@razvanpapadopol razvanpapadopol deleted the add/gutenboarding-new-tracking-events branch May 13, 2020 13:00
@a8ci18n
Copy link

a8ci18n commented May 22, 2020

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/3652236

Hi @razvanpapadopol, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include this string: Choose design

Thank you in advance!

@a8ci18n
Copy link

a8ci18n commented May 28, 2020

Translation for this Pull Request has now been finished.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Tracks Metrics & Monitoring Capturing analytics about user behavior on WordPress.com. [Goal] New Onboarding previously called Gutenboarding
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants