-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃殌 Move ads variables to amp-analytics #25113
馃殌 Move ads variables to amp-analytics #25113
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. One last request. Can we please add the three variables to our integration test at test-amp-analytics.js
Thanks!
57dd441
to
48a87b2
Compare
48a87b2
to
ddf34a7
Compare
@@ -63,14 +66,19 @@ describe('amp-analytics', function() { | |||
document.cookie = '_cid=;expires=' + new Date(0).toUTCString(); | |||
}); | |||
|
|||
it('should send request', () => { | |||
it.only('should send request', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove : )
@@ -50,7 +50,10 @@ describe('amp-analytics', function() { | |||
"cid": "\${clientId(_cid)}", | |||
"loadend": "\${navTiming(loadEventEnd)}", | |||
"default": "\$DEFAULT( , test)", | |||
"cookie": "\${cookie(test-cookie)}" | |||
"cookie": "\${cookie(test-cookie)}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the file that we need to add a <p>Hello World</p>
to get FIRST_CONTENTFUL_PAINT
value? Asking because I think this is the one.
52b2820
to
b11689b
Compare
b11689b
to
1576daf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -75,15 +75,6 @@ <h1 id="top">AMP Analytics</h1> | |||
<p> | |||
Integer in felis at lacus mattis facilisis. Curabitur tincidunt, felis porttitor mollis finibus, tortor elit elementum dolor, vel vulputate lorem dui id ante. Vivamus in velit at lectus blandit gravida vitae quis arcu. Nam et magna magna. Fusce condimentum diam lacus, ac ullamcorper purus malesuada eu. Mauris ullamcorper elit et venenatis faucibus. Nullam lobortis molestie purus quis pellentesque. Sed at libero id nisi rhoncus tincidunt. Praesent vestibulum vehicula tristique. Etiam rutrum, nunc id porta interdum, nulla nisi molestie leo, at fermentum justo dolor at lorem. Duis in egestas sapien. | |||
</p> | |||
<amp-video |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: since you removed amp-video, you may also want to remove amp-video-0.1.js
* master: (181 commits) 馃彈 Ensure valid flag usage for `gulp` tasks (ampproject#26814) build-system: Fix autocomplete error response (ampproject#26824) application/json is ab allowed type for <script> (ampproject#26815) 馃毊 Removing amp-consent-v2 experiment logic (ampproject#26162) Fix more arrow functions that are passed in as "constructors" (ampproject#26795) Variable substitution tester (ampproject#26695) Turn on restrict fullscreen canary (ampproject#26766) Mock variableService getMacros (ampproject#26300) Sync from Google (ampproject#26805) Sync from Google (ampproject#26803) Move video_state macro (ampproject#26212) 馃殌 Move ads variables to amp-analytics (ampproject#25113) Sync from Google (ampproject#26800) Sync from Google (ampproject#26798) Sync from Google (ampproject#26792) Another set of example.com change (ampproject#26753) Add PWA multidoc loader to examples (ampproject#26680) 馃悰Check for window null before creating tracking pixel (ampproject#26749) trying to update Sauce timeouts (ampproject#26737) 馃悰Fixes swipe to dismiss badly ordered swipes on `amp-lightbox-gallery` (ampproject#26788) ... # Conflicts: # extensions/amp-accordion/amp-accordion.md # extensions/amp-bind/amp-bind.md
Project Tracker: #26091
Move FIRST_CONTENTFUL_PAINT, FIRST_VIEWPORT_READY, MAKE_BODY_VISIBLE macros and their tests