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

Clarify behavior of substitutions with amp-analytics #1302

Closed
rudygalfi opened this issue Jan 5, 2016 · 7 comments
Closed

Clarify behavior of substitutions with amp-analytics #1302

rudygalfi opened this issue Jan 5, 2016 · 7 comments

Comments

@rudygalfi
Copy link
Contributor

It appears that several substitutions are default-defined to be used in the amp-analytics variable syntax.

For instance, this ping to send a page view ID can be constructed in two ways: ?pvid=PAGE_VIEW_ID or ?pvid=${pageViewId} since the variable pageViewId inherits the value of PAGE_VIEW_ID by default.

The documentation should clarify how variables can be introduced and used in amp-analytics and talk about any default values inherited from the more general substitution scheme.

cc @avimehta

@Meggin
Copy link
Contributor

Meggin commented Feb 22, 2016

Drafted two docs on variables in this PR (including a doc specifically on variable precedence): ampproject/amp.dev#46

@Meggin
Copy link
Contributor

Meggin commented Feb 25, 2016

Analytics docs have landed. Variable substitution is covered throughout the docs, as well as specifically described in here: https://www.ampproject.org/docs/guides/analytics/analytics_basics.html#variable-substitution and in here: https://www.ampproject.org/docs/guides/analytics/deep_dive_analytics.html#variable-substitution-ordering.

Do you need more, or can we close?

@rudygalfi
Copy link
Contributor Author

I want something that makes it super-clear that VARIABLE_NAME as a substitution is the same thing as ${variableName} as an amp-analytics var.

Another way to talk about this is how https://github.com/ampproject/amphtml/blob/master/extensions/amp-analytics/analytics-vars.md is very redundant with https://github.com/ampproject/amphtml/blob/master/spec/amp-var-substitutions.md and there's nothing talking about how these relate to each other. It might be nice to have a single file that lists the value that's provided ("Screen height") and shows it both as a substitution ("SCREEN_HEIGHT") and as the variable available in amp-analytics ("screenHeight").

@avimehta
Copy link
Contributor

Note that the two lists may be different. Look for requestCount. Over long term, I imagine they will diverge as amp-analytics adds support for vars that the platform may not understand. (No concrete examples yet but if we need to report on viewability of certain elements on page, we would do that calculation in amp-analytics and not in var-substitution).

@rudygalfi
Copy link
Contributor Author

Agree. In that case, the platform (substitution) column will just say "N/A". I think by pulling these into the same file, we may the relationship more understandable and the documentation more maintainable.

@rudygalfi rudygalfi modified the milestones: M2, Backlog Mar 4, 2016
@avimehta
Copy link
Contributor

@Meggin Any progress on this? I will need this for an upcoming feature. If you haven't started and or can't, I'll be happy to take this over..

@rudygalfi
Copy link
Contributor Author

@avimehta I suspect it will be best for you to take it from here. cc @bpaduch

@avimehta avimehta self-assigned this Oct 5, 2016
Meggin pushed a commit that referenced this issue Nov 3, 2016
…5576)

* Combine amp-analytics var docs with var substitutions doc re: #1302

* Updated non-supported vars + format changes for amp-analytics vars

* Updates per Avi's feedback

* Updated descriptions for amp-carousel vars
ruturajv-media-net pushed a commit to ruturajv-media-net/amphtml that referenced this issue Nov 4, 2016
…tion rewrite (ampproject#5982)

* Update amp-ad-network-doubleclick-impl.md

* Reverting doc changes

The previous change was made in error (wrong branch).

* Initial changes, need to update tests

* Start fixing tests

* Fix unit test coverage

* revert a4a examples

Introduce amp-auto-id attribute to AMP element. (ampproject#5973)

* Introduce amp-auto-id attribute to AMP element.

* Fix tests

* Address comments

* Address comments

* revert

amp-live-list.md polling clarification (ampproject#5991)

Only report 1% of errors if a page has non-AMP JS (ampproject#5994)

Fixes ampproject#5732

Allow for hosted testing to override where third party frame is retrieved from (ampproject#5890)

* Allow for hosted testing to override where third party frame is retrieved from

* test my own site

* fix ava tests

Remove unsupported query selector feature (ampproject#5999)

Workaround for misbehaving webview viewer (ampproject#6001)

A webview viewer from Google is seing origins of the form `www.google.com`.

For the time being we accept these as they trigger errors in our error reporting that aren't useful. As this form of origins can only occur with webviews that have full control over the page via JS injection anyway, this change should make no difference for normal web usage.

A4A integration tests (ampproject#5812)

* First draft of full integration tests for A4A.

* First draft of full integration tests for A4A.

* Got tests of exception conditions working.  Refactored some common stuff to utils.js.

* Refactored XDom and friendly iframe tests to helpers.

* Added tests.

* Resolved merge conflicts.

* Centralized visibility check and updated visibility test.

* Moved all A4A tests to using central visibility test.

* Lint fixes.

* Changed custom expectations to local functions.

* Replaced stringToArrayBuffer with utf8Encode.

* Fixed visibility assertion.

* Lint fixes.

* Added a comment (partly in order to get Travis to re-run).

* Changes in response to reviews.

* Rebased and fixed small rebase error.

Fixes an issue where ads are not correctly centered on certain platforms. (ampproject#6003)

* Centralized creative centering in doubleclick.js, and changed css 'transform' to a more commonly supported property.

* Added check for existence and changed to plain old 'transform'.

* Now asserting the existence of the container.

amp-sticky-ad close button new style (ampproject#5979)

* unit style

* fix css

* fix test

* rebase

* fix z-index

Other JS errors: Use startsWith (ampproject#6006)

Fixes ampproject#5994 (comment).

Fix log calls without TAGNAME (ampproject#6005)

My presubmit was a bit too lenient, and too strict at the same time.

refactor amp-ad.css (ampproject#5992)

Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 (ampproject#5576)

* Combine amp-analytics var docs with var substitutions doc re: ampproject#1302

* Updated non-supported vars + format changes for amp-analytics vars

* Updates per Avi's feedback

* Updated descriptions for amp-carousel vars

add version parameter to AMP.extension signature (ampproject#5989)

* add version parameter to AMP.extension signature

* apply recommendations

* remove TODO

Improve error reporting (ampproject#6019)

- Include the current `location.hash`. We are missing it, because it is not included in the referrer.
- Include previously recorded errors, so it is easier to identify follow on errors.
- Kill the 2000 char limit as it isn't important for AMP's target browsers.

Include the originalHash (ampproject#6020)

We remove the hash in embedding mode, so the original code doesn't actually work.

Fix error: "Can't find variable: TextDecoder"  (ampproject#6011)

* Update amp-ad-network-doubleclick-impl.md

* Reverting doc changes

The previous change was made in error (wrong branch).

* Initial changes, need to update tests

* Start fixing tests

* Fix unit test coverage

* revert a4a examples

* Include self (window) when verifying TextDecoder/TextEncoder existence

* fix upstream conflict

* PR feedback

* PR feedback
ruturajv-media-net pushed a commit to ruturajv-media-net/amphtml that referenced this issue Nov 4, 2016
Update ads.amp.html

Modify A4A AMP Creative to use ampRuntimeUtf16CharOffsets from validation rewrite (ampproject#5982)

* Update amp-ad-network-doubleclick-impl.md

* Reverting doc changes

The previous change was made in error (wrong branch).

* Initial changes, need to update tests

* Start fixing tests

* Fix unit test coverage

* revert a4a examples

Introduce amp-auto-id attribute to AMP element. (ampproject#5973)

* Introduce amp-auto-id attribute to AMP element.

* Fix tests

* Address comments

* Address comments

* revert

amp-live-list.md polling clarification (ampproject#5991)

Only report 1% of errors if a page has non-AMP JS (ampproject#5994)

Fixes ampproject#5732

Allow for hosted testing to override where third party frame is retrieved from (ampproject#5890)

* Allow for hosted testing to override where third party frame is retrieved from

* test my own site

* fix ava tests

Remove unsupported query selector feature (ampproject#5999)

Workaround for misbehaving webview viewer (ampproject#6001)

A webview viewer from Google is seing origins of the form `www.google.com`.

For the time being we accept these as they trigger errors in our error reporting that aren't useful. As this form of origins can only occur with webviews that have full control over the page via JS injection anyway, this change should make no difference for normal web usage.

A4A integration tests (ampproject#5812)

* First draft of full integration tests for A4A.

* First draft of full integration tests for A4A.

* Got tests of exception conditions working.  Refactored some common stuff to utils.js.

* Refactored XDom and friendly iframe tests to helpers.

* Added tests.

* Resolved merge conflicts.

* Centralized visibility check and updated visibility test.

* Moved all A4A tests to using central visibility test.

* Lint fixes.

* Changed custom expectations to local functions.

* Replaced stringToArrayBuffer with utf8Encode.

* Fixed visibility assertion.

* Lint fixes.

* Added a comment (partly in order to get Travis to re-run).

* Changes in response to reviews.

* Rebased and fixed small rebase error.

Fixes an issue where ads are not correctly centered on certain platforms. (ampproject#6003)

* Centralized creative centering in doubleclick.js, and changed css 'transform' to a more commonly supported property.

* Added check for existence and changed to plain old 'transform'.

* Now asserting the existence of the container.

amp-sticky-ad close button new style (ampproject#5979)

* unit style

* fix css

* fix test

* rebase

* fix z-index

Other JS errors: Use startsWith (ampproject#6006)

Fixes ampproject#5994 (comment).

Fix log calls without TAGNAME (ampproject#6005)

My presubmit was a bit too lenient, and too strict at the same time.

refactor amp-ad.css (ampproject#5992)

Combine amp-analytics var docs with var substitutions doc re: ampproject#1302 (ampproject#5576)

* Combine amp-analytics var docs with var substitutions doc re: ampproject#1302

* Updated non-supported vars + format changes for amp-analytics vars

* Updates per Avi's feedback

* Updated descriptions for amp-carousel vars

add version parameter to AMP.extension signature (ampproject#5989)

* add version parameter to AMP.extension signature

* apply recommendations

* remove TODO

Improve error reporting (ampproject#6019)

- Include the current `location.hash`. We are missing it, because it is not included in the referrer.
- Include previously recorded errors, so it is easier to identify follow on errors.
- Kill the 2000 char limit as it isn't important for AMP's target browsers.

Include the originalHash (ampproject#6020)

We remove the hash in embedding mode, so the original code doesn't actually work.

Fix error: "Can't find variable: TextDecoder"  (ampproject#6011)

* Update amp-ad-network-doubleclick-impl.md

* Reverting doc changes

The previous change was made in error (wrong branch).

* Initial changes, need to update tests

* Start fixing tests

* Fix unit test coverage

* revert a4a examples

* Include self (window) when verifying TextDecoder/TextEncoder existence

* fix upstream conflict

* PR feedback

* PR feedback
@ghost ghost closed this as completed Nov 7, 2016
Lith pushed a commit to Lith/amphtml that referenced this issue Dec 22, 2016
…ect#1302 (ampproject#5576)

* Combine amp-analytics var docs with var substitutions doc re: ampproject#1302

* Updated non-supported vars + format changes for amp-analytics vars

* Updates per Avi's feedback

* Updated descriptions for amp-carousel vars
Lith pushed a commit to Lith/amphtml that referenced this issue Dec 22, 2016
…ect#1302 (ampproject#5576)

* Combine amp-analytics var docs with var substitutions doc re: ampproject#1302

* Updated non-supported vars + format changes for amp-analytics vars

* Updates per Avi's feedback

* Updated descriptions for amp-carousel vars
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants