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

Site Editor Tracking: Add events for selecting content items in the navigation sidebar #54385

Merged
merged 6 commits into from
Jul 14, 2021

Conversation

david-szabo97
Copy link
Contributor

@david-szabo97 david-szabo97 commented Jul 8, 2021

Changes proposed in this Pull Request

  • Track selecting content item in the navigation sidebar
    • Event name: wpcom_block_editor_nav_sidebar_item_edit
    • Event properties:
      • item_type - type of the selected item (page, post, category, etc.)
      • item_slug - slug of the selected item

Note setPageis called twice due to a bug in Gutenberg: WordPress/gutenberg#33286

Testing instructions

  • Open Site Editor
  • Open navigation sidebar
  • Change to a content item
  • Make sure event is tracked

Make sure E2E tests are passing.

Related to #

@david-szabo97 david-szabo97 self-assigned this Jul 8, 2021
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 8, 2021
@david-szabo97 david-szabo97 requested review from scinos and a team as code owners July 8, 2021 12:18
@matticbot
Copy link
Contributor

matticbot commented Jul 8, 2021

@matticbot
Copy link
Contributor

This PR modifies the release build for wpcom-block-editor

To test your changes on WordPress.com, run install-plugin.sh wpcom-block-editor add/site-editor-track-navigation-content on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-l4k-p2

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

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

This is looking good so far and tests are working well. just a couple comments.

Comment on lines +465 to +466
item_type: type,
item_slug: slug,
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems when we select a 'category' page we just get item_slug with the category name and no item_type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have that information from the args. 😢 Used a selector to check the active menu. Type is determined based on that in this case.

Comment on lines 994 to 995
// TODO: Change this to 1 once https://github.com/WordPress/gutenberg/pull/33286 fix is deployed
assert.strictEqual( editEvents.length, 2 );
Copy link
Contributor

Choose a reason for hiding this comment

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

Another idea might be to debounce the function that sends the event with a very small time window so only 1 event sends. Although, this double-dispatch should definitely be fixed anyways and will solve this problem.

Copy link
Contributor

Choose a reason for hiding this comment

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

@david-szabo97
Copy link
Contributor Author

Anyone planning to run e2e tests, make sure to comment this line:

await driverHelper.clickWhenClickable( this.driver, () => notice );

Latest Gutenberg version introduced a regression, showing the snackbar below the inserter, which prevents us from clicking on the notice to dismiss it.

@matticbot
Copy link
Contributor

This PR modifies the release build for notifications

To test your changes on WordPress.com, run install-plugin.sh notifications add/site-editor-track-navigation-content on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-elI-p2

@matticbot
Copy link
Contributor

This PR modifies the release build for editing-toolkit

To test your changes on WordPress.com, run install-plugin.sh editing-toolkit add/site-editor-track-navigation-content on your sandbox.

To deploy your changes after merging, see the documentation: PCYsg-mMA-p2

@mattwiebe
Copy link
Contributor

Is it expected that wpcom_block_editor_nav_sidebar_item_edit would be called upon the Site Editor upon first load?

{
  "editor_type": "site",
  "item_type": "template",
  "item_id": "pub/tt1-blocks//index",
  "blog_id": 34321322,
  "site_type": "simple",
  "user_locale": "en"
}

The rest of this may be totally expected but I haven't been following the stats work so I had some questions about expected behaviour. Since I was debugging on wpcom-block-editor:analytics:tracks, I noticed that after I clicked on a page to edit within the Site Editor ( wpcom_block_editor_nav_sidebar_item_edit 👍 ) that I got some other events that may or may not be expected?

Screen Shot 2021-07-12 at 15 38 03

Basically what happens is that, after the page is selected, Gutenberg repaints its editor canvas with the blocks of the selected page. This winds up callind wpcom_block_inserted even though these aren't being inserted explicitly by the user, so that could muddy our stats a bit.

@david-szabo97
Copy link
Contributor Author

Is it expected that wpcom_block_editor_nav_sidebar_item_edit would be called upon the Site Editor upon first load?

Nope! Fixed it. Though it's quite hacky 😬

The rest of this may be totally expected but I haven't been following the stats work so I had some questions about expected behaviour. Since I was debugging on wpcom-block-editor:analytics:tracks, I noticed that after I clicked on a page to edit within the Site Editor ( wpcom_block_editor_nav_sidebar_item_edit 👍 ) that I got some other events that may or may not be expected?

This is a known issue 😞 We have a workaround for reusable blocks and template parts. But we found out this problem comes up with many other blocks that includes/loads inner blocks.

Copy link
Contributor

@mattwiebe mattwiebe left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes, this looks good and tests properly 🚢

@david-szabo97 david-szabo97 merged commit ffb9619 into trunk Jul 14, 2021
@david-szabo97 david-szabo97 deleted the add/site-editor-track-navigation-content branch July 14, 2021 10:26
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 14, 2021
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.

4 participants