✨ Add Analytics for branching #20496
Conversation
| STORY_PAGE_COUNT: 'storyPageCount', | ||
| STORY_IS_MUTED: 'storyIsMuted', | ||
| STORY_PROGRESS: 'storyProgress', | ||
| STORY_PREVIOUS_PAGE_ID: 'storyPreviosPageId', |
| AUTO_ADVANCE_MEDIA: 'autoAdvanceMedia', | ||
| MANUAL_ADVANCE: 'manualAdvance', | ||
| ADVANCE_TO: 'advanceTo', | ||
| ADVANCE_TO_ADS: 'advanceToAds', |
There was a problem hiding this comment.
nit: this reads like it is saying the advancement was to an ad, where you're really saying the advancement was from an ad with the advance-to attribute. Since publishers don't set the advance-to attribute for ads (it's set by us internally), I'm not sure the advance-to part is necessary to tell them here.
Maybe we can rename this to something like 'manualAdvanceFromAd'?
There was a problem hiding this comment.
Thanks for catching this! You're right that the variable name is misleading.
Done.
| /** | ||
| * @param {AdvancementMode} mode | ||
| */ | ||
| onAdvancementModeStateChange(mode) { |
There was a problem hiding this comment.
I think you can remove this (and the call to it on line 546 of amp-story.js), since we don't want to actually trigger a new analytics ping when the advancement mode changes. Updating the variable service should be sufficient; then when the next analytics trigger fires (e.g. when the story changes pages, or when the user unmutes the story, or when the user hits the bookend, etc), the variables will be populated because of the rest of what you've done here.
There was a problem hiding this comment.
Thanks for taking the time to explain this. Makes a lot of sense, and removed.
| pageIndex, | ||
| this.getPageCount(), | ||
| targetPage.element.id, | ||
| oldPage.element.id, |
There was a problem hiding this comment.
Can oldPage be undefined and trigger an error, eg: on first navigation?
There was a problem hiding this comment.
Thanks for catching this, I went ahead and moved the code to avoid this problem.
| if (oldPage.element.hasAttribute(advanceAttr)) { | ||
| this.storeService_.dispatch( | ||
| Action.SET_ADVANCEMENT_MODE, | ||
| AdvancementMode.ADVANCE_TO); |
There was a problem hiding this comment.
Just curious about the product decision: Is this overriding any advancement mode previously set?
For example, if the advancement was media based, are we ok with AUTO_ADVANCE_MEDIA being overriden by ADVANCE_TO?
There was a problem hiding this comment.
Hmm, actually, good catch. Now that I think of it, I'm not sure ADVANCE_TO is actually an advancement "mode," per se. It would still be a manual advancement, just to the page specified by the advance-to attribute.
But, they already know what page the advancement was to, from the page ID/index. So, I'm not sure knowing that it was because of the advance-to attribute tells them any new information?
There was a problem hiding this comment.
Good point. The new page id and old page id are available, so it'd make sense to keep the "manual", "media based", or "time based" information.
Edit: just realized I'm paraphrasing your comment, sorry about that, it actually took a few minutes to understand all the context
… manual advancing
There was a problem hiding this comment.
Could you please make sure your implementation also works for the desktop pagination buttons (https://github.com/ampproject/amphtml/blob/master/extensions/amp-story/1.0/pagination-buttons.js#L146) and the embedded components (https://github.com/ampproject/amphtml/blob/master/extensions/amp-story/1.0/amp-story-embedded-component.js#L768)?
| targetPage.element.id, | ||
| oldPage.element.id, | ||
| targetPage.getNextPageId() === null /* isFinalPage */ | ||
| ); |
There was a problem hiding this comment.
Moving this piece of code here changes the behavior of the analytics: now it won't get triggered for the first page, when a user loads the story.
I think we can keep the code where it was before, and just make sure we don't cause any error when oldPage is not defined.
| * @param {number} pageIndex | ||
| * @param {number} totalPages | ||
| * @param {string} pageId | ||
| * @param {string} previousPageId |
There was a problem hiding this comment.
Nit: previousPageId is nullable, for the first navigation (when the story renders)
There was a problem hiding this comment.
Nit: ?string is the shorthand for string|null if you want :))
Manually tested and seems to be working as intended. |
* WIP adding advacementMode and previousPageId as analytics variables * WIP: made more changes following the onNavigationState update pattern instead * moved around some code, got rid of excess * added tests * tested with helloworld * cleaned code * linted, cleaned more * got rid of changes to examples, removed unecessary changes * fixed typo, oops! * removed analytics trigger, addressed nits * removed analytics test, added variable service test * removed advance_to as an advancement mode, capture other instances of manual advancing * addressed requested changes * ?string shorthand instead of string|null * fixed navigation state tests
With branching, we'd like to be able to track how the user navigates through a story.
This PR adds variables:
#20083