Skip to content

🚀 Move ads variables to amp-analytics#25113

Merged
micajuine-ho merged 9 commits intoampproject:masterfrom
micajuine-ho:move_ads_variables
Feb 14, 2020
Merged

🚀 Move ads variables to amp-analytics#25113
micajuine-ho merged 9 commits intoampproject:masterfrom
micajuine-ho:move_ads_variables

Conversation

@micajuine-ho
Copy link
Copy Markdown
Contributor

@micajuine-ho micajuine-ho commented Oct 17, 2019

Project Tracker: #26091

Move FIRST_CONTENTFUL_PAINT, FIRST_VIEWPORT_READY, MAKE_BODY_VISIBLE macros and their tests

@zhouyx zhouyx self-requested a review October 17, 2019 22:47
Copy link
Copy Markdown
Contributor

@zhouyx zhouyx left a 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!

@micajuine-ho micajuine-ho force-pushed the move_ads_variables branch 5 times, most recently from 57dd441 to 48a87b2 Compare December 20, 2019 00:12
@micajuine-ho micajuine-ho requested a review from zhouyx December 20, 2019 00:15
});

it('should send request', () => {
it.only('should send request', () => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove : )

"loadend": "\${navTiming(loadEventEnd)}",
"default": "\$DEFAULT( , test)",
"cookie": "\${cookie(test-cookie)}"
"cookie": "\${cookie(test-cookie)}",
Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown
Contributor

@zhouyx zhouyx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

<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
Copy link
Copy Markdown
Contributor

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

@micajuine-ho micajuine-ho merged commit feaaddf into ampproject:master Feb 14, 2020
robinvanopstal added a commit to jungvonmatt/amphtml that referenced this pull request Feb 17, 2020
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants