Remove experimental flag from visibility triggers and add documentation. #3512
Conversation
|
@avimehta I see experiment itself is removed, but I don't see where it's cleaned. Assigning to @rudygalfi for docs review. |
| Use this configuration to fire a request when the page becomes visible. No further configuration is required. | ||
| Use this configuration to fire a request when the page becomes visible. The firing of this trigger can be configured using `visibilitySpec`. Configuration properties supported in `visibilitySpec` are: | ||
| - `selector` This property can be used to specify the element to which all the `visibilitySpec` conditions apply. | ||
| - `continuousTimeMin` and `continuousTimeMax` These conditions keep track of the amount of time (any part of) an element has been in the viewport and fire when the time is between the minimum and maximum configured time. The time is expressed in milliseconds. |
There was a problem hiding this comment.
These properties indicate that a request should be fired when (any part of) and element has been within the viewport between the minimum and maximum specified times. The times are expressed in milliseconds.
There was a problem hiding this comment.
Perhaps even clearer:
These properties indicate that a request should be fired when (any part of) and 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.
|
LGTM on docs with a few minor comments |
|
@rudygalfi You LGTM'ed it but pinging you just in case you want to take a second look. @dvoytenko This is ready for your review. |
|
LGTM from my side. |
|
@dvoytenko @avimehta Is this ready to be submitted? It looks like Travis failed. |
|
@rudygalfi looking into the test failures. Will update soon. |
|
The call https://github.com/ampproject/amphtml/blob/master/extensions/amp-analytics/0.1/visibility-impl.js#L243 results in undefined for simple buttons (just text, no image etc.) that are valid in AMP. The next line breaks of course on res.getId(); since res is undefined. I think the same happens for img, links etc. About the only thing that seems to work is "amp-img" with real resource (i.e. if it refers to delete image it still fails). Simple buttons, links and img (not amp-img) etc. should work with this but they don't. |
/cc @rudygalfi
#1297