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

🐛Fix a2a for next article links in amp-next-page #18517

Merged
merged 1 commit into from
Oct 8, 2018

Conversation

sparhami
Copy link

@sparhami sparhami commented Oct 2, 2018

Fixes #18512

@tchirag
Copy link

tchirag commented Oct 8, 2018

Is this removal of Analytics trigger on amp-next-page?

@sparhami
Copy link
Author

sparhami commented Oct 8, 2018

Is this removal of Analytics trigger on amp-next-page?

I believe that the event that triggers for a2a within <amp-next-page> would have triggered before this fix as well, since the code for the event happens before the line with the error. The actual amp to amp navigation would not have happened though.

@sparhami sparhami merged commit cbb5017 into ampproject:master Oct 8, 2018
@sparhami sparhami deleted the next-page branch October 8, 2018 18:45
@tchirag
Copy link

tchirag commented Oct 11, 2018

Is this removal of Analytics trigger on amp-next-page?

I believe that the event that triggers for a2a within <amp-next-page> would have triggered before this fix as well, since the code for the event happens before the line with the error. The actual amp to amp navigation would not have happened though.

Unfortunately with this push, trigger for pageview event on Google Analytics has stopped on amp-next-page. To explain, pageview event gets triggered on first page however doesnt get triggered on 2nd and 3rd page which are loaded using amp-next-page. Is this expected outcome?

@ampproject ampproject deleted a comment Oct 11, 2018
@ampproject ampproject deleted a comment Oct 11, 2018
@ampproject ampproject deleted a comment Oct 11, 2018
@ampproject ampproject deleted a comment Oct 11, 2018
@sparhami
Copy link
Author

sparhami commented Oct 11, 2018

Unfortunately with this push, trigger for pageview event on Google Analytics has stopped on amp-next-page. To explain, pageview event gets triggered on first page however doesnt get triggered on 2nd and 3rd page which are loaded using amp-next-page. Is this expected outcome?

Do you mean an analytics event when the user scrolls and the next page is loaded or when the user clicks on a link (in the recommendation box). To the best of my knowledge, this change should only affect when the user clicks on a link. Are you seeing the missing analytics when you click on one of those links?

@peterjosling do you have any insight as to what may be going on? I'm not familiar with the amp 2 amp navigation flow, could there be some problem there?

@sparhami
Copy link
Author

Hmm, I wonder if this is related to the fact that we strip the <amp-analytics> config from <amp-next-page>. It may be that the analytics was working before because it was a hard page navigation. Now, the event fires to let it know that an a2a navigation happened, then we do an a2a navigation. It could be that nothing is actually listening for that event though.

@tchirag
Copy link

tchirag commented Oct 11, 2018 via email

@sparhami
Copy link
Author

@lannka Are you aware of any issues or changes with <amp-analytics> and/or <amp-next-page> that might prevent the analytics triggering?

@peterjosling
Copy link
Contributor

Analytics are currently disabled in child documents. #15807

@tchirag
Copy link

tchirag commented Nov 14, 2018

Even creating DFP Ad Units isnt possible currently?

Enriqe pushed a commit to Enriqe/amphtml that referenced this pull request Nov 28, 2018
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

6 participants