Added support for a new trigger on=hidden.#4265
Conversation
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? |
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. |
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