Added support for a new trigger on=hidden.
#4265
Conversation
| - `selector` This property can be used to specify the element to which all the `visibilitySpec` conditions apply. The selector needs to be an css id that points to an [AMP extended element](https://github.com/ampproject/amphtml/blob/master/spec/amp-tag-addendum.md#amp-specific-tags). | ||
| - `continuousTimeMin` and `continuousTimeMax` These properties indicate that a request should be fired when (any part of) an element has been within the viewport for a continuous amount of time that is between the minimum and maximum specified times. The times are expressed in milliseconds. Due to semantic restrictions, `continuousTimeMax` only works when the page is being hidden. Hence `continuousTimeMax` is not yet supported. | ||
| - `totalTimeMin` and `totalTimeMax` These properties indicate that a request should be fired when (any part of) an element has been within the viewport for a total amount of time that is between the minimum and maximum specified times. The times are expressed in milliseconds. Due to semantic restrictions, `totalTimeMax` only works when the page is being hidden. Hence `totalTimeMax` is not yet supported. | ||
| - `continuousTimeMin` and `continuousTimeMax` These properties indicate that a request should be fired when (any part of) an element has been within the viewport for a continuous amount of time that is between the minimum and maximum specified times. The times are expressed in milliseconds. |
There was a problem hiding this comment.
The Max vars are the ones that are only valid for hidden, right?
Can you note that here?
There was a problem hiding this comment.
I thought about it a bit more and I think a condition like the following should be valid. that is the reason I removed the text that mentioned that *max conditions are not valid for "on"="visible"
Send a hit if the element is visible at least for 5 seconds but not for more than 1 second at a time.
"continuousTimeMax": 1000,
"totalTimeMin": 5000
Send a hit if the element is visible for not more than 5 seconds in total but at least visible for 1 second continuously.
"continuousTimeMin": 1000,
"totalTimeMax": 5000
ps: I think the examples above are a bit made-up and may not be realistic but let's not limit the usage because we can't think of a use-case right now.
|
Just one small comment, but docs overall LGTM |
|
@dvoytenko Do you mind reviewing this PR? |
| @@ -128,7 +129,7 @@ export class InstrumentationService { | |||
| addListener(config, listener) { | |||
| const eventType = config['on']; | |||
| if (eventType === AnalyticsEventType.VISIBLE) { | |||
| this.createVisibilityListener_(listener, config); | |||
| this.createVisibilityListener_(listener, config, true); | |||
There was a problem hiding this comment.
Please add /* arg */ true
|
@avimehta Generally things look alright. One thing is that the use of non-throttled scroll listener here might be pretty expensive. I'm cc-ing @jridgewell from the initial review. Is the non-throttled scroll listener absolutely necessary here? Did we debug the performance of this? |
|
rebased and Fixed #4361. ptal. |
| @@ -131,7 +132,7 @@ export class InstrumentationService { | |||
| addListener(config, listener) { | |||
| const eventType = config['on']; | |||
| if (eventType === AnalyticsEventType.VISIBLE) { | |||
| this.createVisibilityListener_(listener, config); | |||
| this.createVisibilityListener_(listener, config, /* visible */ true); | |||
There was a problem hiding this comment.
Why not pass in AnalyticsEventType.VISIBLE or HIDDEN?
There was a problem hiding this comment.
I think that can be done. Will update the PR. (no negatives I can think of)
There was a problem hiding this comment.
updated the PR. I did not change visibility-impl to also accept the eventType because it didn't make a whole lot of sense. I did update the names and description to clarify what the code is trying to do.
|
LGTM |
|
@dvoytenko Thanks for the review here. I'll need to resolve conflicts with #4072 and then I'll merge. Regarding your our comment about performance of the scroll events, I am open to switching to batched events if performance becomes an issue. Most of the calculations on each scroll have been a few of counter updates and if-else blocks so far. |
The new trigger fires when the viewer is hidden. In addition, visibilitySpec can be used to add additional conditions on when the trigger fires.
* Added support for a new trigger `on=hidden`. The new trigger fires when the viewer is hidden. In addition, visibilitySpec can be used to add additional conditions on when the trigger fires. * variable rename.
* Added support for a new trigger `on=hidden`. The new trigger fires when the viewer is hidden. In addition, visibilitySpec can be used to add additional conditions on when the trigger fires. * variable rename.
The new trigger fires when the viewer is hidden. In addition,
visibilitySpec can be used to add additional conditions on when the
trigger fires.
Fixes #2487 #2564