Render-start analytics event for documents and embeds#7220
Render-start analytics event for documents and embeds#7220dvoytenko merged 12 commits intoampproject:masterfrom
Conversation
spec/amp-var-substitutions.md
Outdated
|
|
||
| #### Time since created | ||
|
|
||
| Provides the time elapsed since the creation of this document or embed. |
There was a problem hiding this comment.
Tests are coming...
There was a problem hiding this comment.
Oh. Wait. You mean unit of time. Got it.
| ``` | ||
|
|
||
| #### Embed render start trigger (`"on": "render-start"`) | ||
| Use this configuration to fire a request when this document's or a specified embed's receive render-start signal. This |
There was a problem hiding this comment.
There are some grammatical issues here.
I think what would really help is to start out by describing what an embed (give ad example) and a document is.
src/service/ampdoc-impl.js
Outdated
| this.win = win; | ||
|
|
||
| /** @private {boolean} */ | ||
| this.isRenderStarted_ = false; |
There was a problem hiding this comment.
Why is this not signal based?
There was a problem hiding this comment.
Do you mean you want to apply the same signals instrument to ampdocs as well? That's probably a good idea. Will do.
There was a problem hiding this comment.
Probably want some kind of Signable interface
bbca26d to
f7d9cec
Compare
| /** | ||
| * Signals that the document has been started rendering. | ||
| * @restricted | ||
| */ |
There was a problem hiding this comment.
any reason why the method is here, and called in style-installer.js?
There was a problem hiding this comment.
There's more logical coming to Resources based on this signal.
There was a problem hiding this comment.
what kind of logic?
and how should this work in amp-inabox?
There was a problem hiding this comment.
I'm working on moving some scheduling here as well.
This should be called and work exactly the same with amp-inabox with one exception: we also want to postMessage render-start which is what you are working on :) Where exactly postMessage belongs in, I'm not sure. I was going to see where you end up putting it. But my first suggestion was to stick it right into amp-inabox.js. That means that we may need a callback from this method, which you can add later if that's what you'd like to do.
| } | ||
| ``` | ||
|
|
||
| For an embed within a document, the trigger includes a selector to specify the embed element: |
There was a problem hiding this comment.
do you want to talk about the definition of "embed element"?
There was a problem hiding this comment.
@cramforce @lannka I rewrote this. Please advise if this is better.
| return true; | ||
| }; | ||
| signals = {}; | ||
| adElement.signal = name => { |
There was a problem hiding this comment.
Not clear why you need to do this.
| ``` | ||
|
|
||
| #### Embed render start trigger (`"on": "render-start"`) | ||
| Use this configuration to fire a request when the document or a specified embed emit `render-start` signal. This |
There was a problem hiding this comment.
emits
But I'd consider switching the language to not use our internal name "signal" and just make this a plain event.
| } | ||
| ``` | ||
|
|
||
| For an embed within a document, the trigger includes a selector to specify the embed element: |
|
Tests have been added. |
* Render-start analytics event * resolve cyclical dependency * restrict render-start * tests and docs * working on docs * DOMReady * redo via signals * tests * Revert * Changes to the docs * tests * lints
* Render-start analytics event * resolve cyclical dependency * restrict render-start * tests and docs * working on docs * DOMReady * redo via signals * tests * Revert * Changes to the docs * tests * lints
render-startevent is emitted by documents and some embeds when it's confirmed that the rendering of the document has been started.