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

✨📈 Add analytics trigger for elements that show a tooltip #25547

Merged
merged 6 commits into from Nov 18, 2019

Conversation

Enriqe
Copy link
Contributor

@Enriqe Enriqe commented Nov 11, 2019

Closes #25545
Closes #24122

Adds story-focus trigger to track whenever an element that opens a tooltip is clicked and story-click-through whenever a tooltip is clicked.

It also adds support for using tagName to target specific elements that can trigger the tooltip.

If the overall implementation / idea looks good I will write the corresponding tests.

Copy link
Contributor

@gmajoulet gmajoulet left a comment

Choose a reason for hiding this comment

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

e2e tests would be cool as a followup :))

extensions/amp-story/1.0/story-analytics.js Outdated Show resolved Hide resolved
extensions/amp-story/1.0/story-analytics.js Outdated Show resolved Hide resolved
if (
tagName &&
pageDetails['tagName'] &&
tagName.toLowerCase() !== pageDetails['tagName']
Copy link
Contributor

Choose a reason for hiding this comment

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

Also .toLowerCase() the pageDetails one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am doing that on the updateDetails method in story-analytics.js when reading the element. Would you prefer it to be here instead?

@@ -104,15 +114,43 @@ <h1>Page Two</h1>
<amp-story-page id="p3">
<amp-story-grid-layer template="vertical">
<h1>Page Three</h1>
<a href="google.com">click me</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that I understand. But in your example, you are listening the story-focus/story-click-through event of all anchor tag. Do all anchor tag fire these two events, or it is only anchor tag created by embedded components and affiliate links?

Would this anchor tag fire those two events? Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only anchor tags created by embedded components and affiliate links will fire this event, since they are the only type of anchor tags that will show a tooltip in stories currently

Copy link
Contributor

Choose a reason for hiding this comment

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

Yuxuan makes a good point here, what prevents this link within a grid-layer to trigger the regular click analytics event? Since it has custom events, it should not trigger this default "click". Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I added a stopPropagation() where appropriate so that these will only be handled by our custom events, and not by the regular click analytics event.

@Enriqe
Copy link
Contributor Author

Enriqe commented Nov 13, 2019

Friendly ping @zhouyx

this.tooltip_.addEventListener(
'click',
() =>
this.analyticsService_.triggerEvent(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to stop propagation here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm not for now since it's inside a shadow root. It won't be recognized by the selector. If we were to support selector inside a shadow root then we would have to do that. I'll just add it, just in case :) PTAL

Copy link
Contributor

@zhouyx zhouyx left a comment

Choose a reason for hiding this comment

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

approved with one nit. Thanks.

extensions/amp-story/1.0/story-analytics.js Show resolved Hide resolved
extensions/amp-story/1.0/story-analytics.js Outdated Show resolved Hide resolved
@Enriqe
Copy link
Contributor Author

Enriqe commented Nov 13, 2019

Thanks @zhouyx! Good points about the variable names

@Enriqe Enriqe merged commit fe19095 into ampproject:master Nov 18, 2019
@Enriqe Enriqe deleted the analytics-triggers1 branch November 18, 2019 18:30
micajuine-ho pushed a commit to micajuine-ho/amphtml that referenced this pull request Dec 27, 2019
…#25547)

* support for focused and click thru states

* support affiliate links

* types

* stop propagation on click events within affiliate links and embedded components

* add stop propagation in tooltip click

* renaming
@deeksha-agarwal
Copy link

@Enriqe is there any documentation available for this implementation. It will be very helpful. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
7 participants