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

[amp-story] Fix story_page_count #33948

Merged
merged 1 commit into from
Apr 22, 2021

Conversation

Enriqe
Copy link
Contributor

@Enriqe Enriqe commented Apr 22, 2021

The storyPageCount in <amp-story-auto-analytics> wasn't actually sending storyPageCount. The problem is that the AMP visible (also ini-load, etc..) trigger goes through the non-story analytics service, which will not contain any of the story-specific variables (like storyPageCount).

This change introduces a StoryAnalyticsEvent.STORY_CONTENT_LOADED trigger that fires once the story is loaded. By using this event, we make sure that the story-specific variables that we need are there when the event is fired.

@amp-owners-bot
Copy link

Hey @gmajoulet! These files were changed:

extensions/amp-story-auto-analytics/0.1/auto-analytics-configs.js
extensions/amp-story/1.0/amp-story.js
extensions/amp-story/1.0/story-analytics.js

Hey @mszylkowski! These files were changed:

extensions/amp-story-auto-analytics/0.1/auto-analytics-configs.js

Hey @newmuis! These files were changed:

extensions/amp-story/1.0/amp-story.js
extensions/amp-story/1.0/story-analytics.js

@Enriqe Enriqe merged commit c78f3bb into ampproject:main Apr 22, 2021
@gmajoulet
Copy link
Contributor

Just curious, how was this bug reported?

@Enriqe
Copy link
Contributor Author

Enriqe commented Apr 22, 2021

It wasn't, I was just playing around with the component and noticed this specific metric wasn't working correctly @gmajoulet

@gmajoulet
Copy link
Contributor

Good catch! Can you explain how it's failing? Was it sometimes working, sometimes sending 0/null/undefined/not being sent at all?
We should communicate this to people who built dashboards using amp-story-auto-analytics.

@Enriqe
Copy link
Contributor Author

Enriqe commented Apr 22, 2021

The el (event_label) value was empty always. And sounds good, will let them know.

@calebcordry
Copy link
Member

Ugh try to fix one bug, but cause another :( I thought we manually tested this? Lets make sure we have a test with all the story specific vars?

ampprojectbot pushed a commit that referenced this pull request Apr 22, 2021
(cherry picked from commit c78f3bb)
rochapablo pushed a commit to rochapablo/amphtml that referenced this pull request Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants