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

Amp-imgur : Implement imgur embed (#9405) #9434

Merged
merged 23 commits into from
Jun 1, 2017
Merged

Amp-imgur : Implement imgur embed (#9405) #9434

merged 23 commits into from
Jun 1, 2017

Conversation

techhtml
Copy link
Contributor

@techhtml techhtml commented May 19, 2017

Amp-imgur : Implement imgur embed (#9405)

This gives developers the ability to easily embed a imgur in their AMP pages.

  • Implements imgur embed

Closes #9405

Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Does imgur send any resize information?


/** @override */
renderOutsideViewport() {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

This might not be needed, depending on how resource hungry the embed is... even Facebook can render outside viewport.

/** @override */
buildCallback() {
this.imgurId_ = user().assert(
this.element.getAttribute('data-imgurid'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change this to data-imgur-id?

Copy link
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

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

This won't be a useful component unless it handles resizing. Since imgur (similar to instagram) adds extra text and padding around the image, there is no way for the author to know the width and height to set here.

amp-imgur should be able to resize the iframe after imgur adds the extra chrome around the image. This can be done two ways:
1- imgur's iframe might be sending messages to the parent notifying of size changes, if that's the case, we can use this code and just listen for that message and request a resize.
2- use imgur's SDK to embed the image in an AMP 3p frame and measure and send a resize request (similar to amp-instagram)

Idea is for user to specify the width and height of the image only and for component to resize properly to account for the chrome around the image. Without handling resize, this is no different than amp-iframe src=imgur.com/id/embed

@techhtml
Copy link
Contributor Author

@aghassemi I also think if this component can't resize, it's not helpful for other users.
I'll rebuild this one use SDK for imgur and amp 3p iframes.

Thank you.

@techhtml
Copy link
Contributor Author

I've added resize logic in amp-imgur.
but they have a problems.

  1. they are resizing but only one time happen when first loaded.
  2. For check loaded iframes, I've use setinterval but looks it's not good. How can i fix it?

@aghassemi
Copy link
Contributor

@techhtml looks like Imgur does send resize messages back to parent, so the old approach of using iframe to point to imgur would work fine, we just need to listen for the resize message and do a attemptchangeHeight()

Checkout https://codepen.io/aghassemi/pen/wdOGwB?editors=1111 with console open, note that imgur sends a {isTrusted: true, data: "{"message":"resize_imgur","href":"https://imgur.co…ub=true","height":509,"width":540,"context":true}", origin: "https://imgur.com", lastEventId: "", source: Window…} message after load.

So what we need to do is:
1- revert to the old iframe code you had
2- install a message listener in amp-imgur's layoutCallback
3- when received resize_imgur call this.attemptChangeHeight with the width/height that comes from imgur.
4- remove the message listener in amp-imgur's unlayoutCallback

amp-playbuzz.js can be a good reference as it does exactly this.

@techhtml
Copy link
Contributor Author

Thank you! @aghassemi I have a Q about my issues #9405.
Cramforce said to me it seems better if just loads an image with some Imgur branding.
Actually, it's not bad. but that logic only work when Imgur have the single image.

So, What is the best for Imgur?
Now I start tried to replace 3p-iframe to iframe and add message listener and resize that.

@aghassemi
Copy link
Contributor

@techhtml we chatted about this in today's review meeting (#8902). Decided ideal solution is normal iframe if we get resize messages from imgur and since we do, let's go with that approach.

@techhtml
Copy link
Contributor Author

@aghassemi Thank you for your answer! I create that use iframes.

this.applyFillContent(iframe);

listenFor(iframe, 'embed-size', data => {
this.changeHeight(data.height);
Copy link
Contributor

Choose a reason for hiding this comment

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

#attemptChangeSize, please.


/** @override */
renderOutsideViewport() {
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove this method.

3p/imgur.js Outdated
* @param {string} scriptSource
*/
function getImgurScript(global, cb) {
loadScript(global, "https://s.imgur.com/min/embed.js", function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the function() {} and just pass cb.


/** @private */
hadleImgurMessages_(event) {
const data = isObject(event.data) ? event.data : tryParseJson(event.data);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please check that the event.source is the iframe's contentWindow

const height = data.height;

this.getVsync().measure(() => {
if (this.iframe_ && this.iframe_.offsetHeight !== height) {
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to do this checking, #attemptChangeHeight will do it for you.

Copy link
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

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

Thanks @techhtml few comments, nothing major. Thanks for iterating on this to deliver the ideal solution.

this.iframe_ = null;

/** @private {?Promise} */
this.iframePromise_ = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to keep a reference to the promise. just return this.loadPromise(iframe); in layoutCallback

/** @private */
hadleImgurMessages_(event) {
const data = isObject(event.data) ? event.data : tryParseJson(event.data);
if(data.message == 'resize_imgur') {
Copy link
Contributor

Choose a reason for hiding this comment

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

if (!data) {
 return;
}

before this line

# limitations under the license.
#

tags: { # amp-imgur
Copy link
Contributor

Choose a reason for hiding this comment

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

attr_lists: "extended-amp-global"
spec_url: "https://www.ampproject.org/docs/reference/components/amp-hello-world"
amp_layout: {
supported_layouts: RESPONSIVE
Copy link
Contributor

Choose a reason for hiding this comment

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

      supported_layouts: FILL
      supported_layouts: FIXED
      supported_layouts: FIXED_HEIGHT
      supported_layouts: FLEX_ITEM
      supported_layouts: NODISPLAY
      supported_layouts: RESPONSIVE

</tr>
<tr>
<td class="col-fourty"><strong><a href="https://www.ampproject.org/docs/guides/responsive/control_layout.html">Supported Layouts</a></strong></td>
<td>responsive</td>
Copy link
Contributor

Choose a reason for hiding this comment

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

fill, fixed, fixed-height, flex-item, nodisplay, responsive

gulpfile.js Outdated
@@ -113,6 +114,7 @@ declareExtension('amp-springboard-player', '0.1', false);
declareExtension('amp-sticky-ad', '0.1', true);
declareExtension('amp-sticky-ad', '1.0', true);
declareExtension('amp-selector', '0.1', true);
declareExtension('amp-imgur', '0.1', false);
Copy link
Contributor

Choose a reason for hiding this comment

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

remove, already declared above

/** @override */
buildCallback() {
this.imgurid_ = user().assert(
this.element.getAttribute("data-imgur-id"),
Copy link
Contributor

Choose a reason for hiding this comment

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

single quote

this.attemptChangeHeight(height)
.catch(() => {});
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

;

/** @private */
hadleImgurMessages_(event) {
const data = isObject(event.data) ? event.data : tryParseJson(event.data);
if(data.message == 'resize_imgur') {
Copy link
Contributor

Choose a reason for hiding this comment

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

if ( (space)

/** @override */
unlayoutCallback() {
if (this.iframe_) {
removeElement(this.iframe_);
Copy link
Contributor

Choose a reason for hiding this comment

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

removeElement needs to be imported from ../../../src/dom

Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

Thank you for contributing a new component! I've reviewed only the validator portion of the PR.

In addition to those changes, it would be nice to add a test file for validation. It comprises of two files:
amp-imgur/0.1/test/validator-amp-imgur.html
amp-imgur/0.1/test/validator-amp-imgur.out

For more information on the test files see https://github.com/ampproject/amphtml/blob/master/contributing/component-validator-rules.md#test-files

tags: { # amp-imgur
html_format: AMP
tag_name: "SCRIPT"
satisfies: "amp-imgur extension .js script"
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put requires: "amp-imgur" after this satisfies.

allowed_versions: "latest"
}
attr_lists: "common-extension-attrs"
spec_name: "amp-imgur extension .js script"
Copy link
Contributor

Choose a reason for hiding this comment

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

Everything from spec_name down in this tag is not necessary as it is handled by extension_spec. To be clear, please remove:

  spec_name: "amp-imgur extension .js script"
  satisfies: "amp-imgur extension .js script"
  requires: "amp-imgur"
  mandatory_parent: "HEAD"
  unique: true
  extension_unused_unless_tag_present: "amp-imgur"
  attrs: {
    name: "async"
    mandatory: true
    value: ""
  }
  attrs: {
    name: "custom-element"
    mandatory: true
    value: "amp-imgur"
    dispatch_key: true
  }
  attrs: { name: "nonce" }
  attrs: {
    name: "src"
    mandatory: true
    value_regex: "https://cdn\.ampproject\.org/v0/amp-imgur-(latest|0\.1).js"
  }
  attrs: {
    name: "type"
    value: "text/javascript"
  }
  cdata: {
    blacklisted_cdata_regex: {
      regex: "."
      error_message: "contents"
    }
  }
  spec_url: "https://www.ampproject.org/docs/reference/components/amp-imgur"

Copy link
Contributor

Choose a reason for hiding this comment

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

spec_name should also be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@honeybadgerdontcare I remove that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@techhtml I'm still seeing it after attr_lists.
L28: spec_name: "amp-imgur extension .js script"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry, It's my mistake.
I remove that.

mandatory: true
}
attr_lists: "extended-amp-global"
spec_url: "https://www.ampproject.org/docs/reference/components/amp-hello-world"
Copy link
Contributor

Choose a reason for hiding this comment

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

s/amp-hello-world/amp-imgur

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wha was that? s/amp-hello-world/amp-imgur?

Copy link
Contributor

@jridgewell jridgewell May 25, 2017

Choose a reason for hiding this comment

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

Vimtalk for replace "amp-hello-world" with "amp-imgur"

* change some syntax
* change validation rules
* add find event source
event.source != this.iframe_.contentWindow) {
return;
}
if (!event.data || !(isObject(event.data))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be a string, too.

const data = isObject(event.data) ? event.data : tryParseJson(event.data);
if(data.message == 'resize_imgur') {
if (data.message == 'resize_imgur') {
const height = data.height;

this.getVsync().measure(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This vsync call is unnecessary now that we're not reading the outerHeight. We can just call #attemptChangeHeight

Copy link
Contributor

@honeybadgerdontcare honeybadgerdontcare left a comment

Choose a reason for hiding this comment

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

Validation looks good. The test files I mentioned are optional.

Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

LGTM

if (this.iframe_) {
removeElement(this.iframe_);
this.iframe_ = null;
this.iframePromise_ = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Delete this line.

satisfies: "amp-imgur"
requires: "amp-imgur extension .js script"
attrs: {
name: "data-imgurid"
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed several of your examples use data-imgur-id and not data-imgurid, so lets update this to data-imgur-id here on line 35.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@honeybadgerdontcare I change that right now! Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! All that remains are any changes @aghassemi requested and for @aghassemi to review/approve those changes. Given the long weekend I don't think that will happen till Tuesday.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, it's cool! Thanks a lot and best regards.

Copy link
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

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

LGTM @techhtml will merge as soon as CI passes. Thanks!

@techhtml
Copy link
Contributor Author

I have a question about travis CI.
Travis has errors in pre_build_checks, I can't see errors but when before that error also happen because of a dead link of tps://github.com/ampproject/amphtml/blob/master/extensions/amp-imgur/0.1/validator-amp-imgur.protoascii.

Maybe this will add when merged this branch. How can I do for a fix that? I think I can change the file path to my repository but it's not a clear answer.

@honeybadgerdontcare
Copy link
Contributor

@techhtml That is check-links.js being overzealous IMO. I've filed #9628 to get it addressed.

@aghassemi If the other tests pass would it be possible to force merge this despite the check-links.js error? Or should we whitelist this url in check-links.js then after merge remove it from the whitelist?

@aghassemi
Copy link
Contributor

I don't have powers to force merge, let's whitelist or temporarily remove from .md file and readd in separate PR.

honeybadgerdontcare added a commit that referenced this pull request May 31, 2017
For PR #9434, whitelist link to validator-amp-imgur.protoascii.
@honeybadgerdontcare
Copy link
Contributor

@techhtml Lets whitelist the url for now. You'll need to sync your branch to get the latest changes to check-links.js and pull in my change from PR #9630 (not merged yet, so you'll need to make it in your PR).

@techhtml
Copy link
Contributor Author

techhtml commented May 31, 2017

@honeybadgerdontcare I added whitelist.

@honeybadgerdontcare
Copy link
Contributor

@techhtml Thanks! It looks like the current travis failure may be due to another PR #9575. I've asked them to look into it.

@honeybadgerdontcare
Copy link
Contributor

travis issue has been fixed. all systems go. merging.

@honeybadgerdontcare honeybadgerdontcare merged commit 482270b into ampproject:master Jun 1, 2017
torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Jun 6, 2017
* Amp-imgur : Implement imgur embed (ampproject#9405)

* amp-imgur test code modified

* amp-imgur validation code changed

* fix amp-imgur lint check

* Remove unused Layour variables

* Test code modified

* Ingegration amp-imgur extension

* [amp-imgur] add resize logic

* Remove 3p iframes and add resize message listener

* Fix some code with reviews
* change some syntax
* change validation rules
* add find event source

* [amp-imgur] remove unnecessary code and add object check

* remove spec_name from validation code

* Remove unnecessary code and add owners file

* Change required tag name  to  in validation file

* data-imgurid -> data-imgur-id

* Add whitelist for amp-imgur

* remove unused AmpImgur import in test file

* remove unused preconnect callback
torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Jun 6, 2017
* Bidtellect amp implementation… (ampproject#9518)

* Bidtellect amp implementation…

* removing renderStartImplemented…

* spaces?

* adding example

* Fix test-amp-ad-network-doubleclick-impl (ampproject#9645)

* Optimizations to speed up Travis PR builds (ampproject#9626)

Travis builds have become slow during busy periods of the day due to various reasons. This PR does the following:

Reorganizes build tasks across fewer build shards
Reduces the number of VMs used by PR and master builds to 2
No longer does a full gulp dist for unit tests (Possible to do after ampproject#9404)
Reduces the total CPU time used for PR builds to < 20 mins
Fixes ampproject#9500
Fixes ampproject#9387

* Animations: full on-action API (ampproject#9641)

* Animations: full on-action API

* review fixes

* Fix Validator tests on Travis, which now uses Node v4 (ampproject#9654)

* Amp-imgur : Implement imgur embed (ampproject#9405) (ampproject#9434)

* Amp-imgur : Implement imgur embed (ampproject#9405)

* amp-imgur test code modified

* amp-imgur validation code changed

* fix amp-imgur lint check

* Remove unused Layour variables

* Test code modified

* Ingegration amp-imgur extension

* [amp-imgur] add resize logic

* Remove 3p iframes and add resize message listener

* Fix some code with reviews
* change some syntax
* change validation rules
* add find event source

* [amp-imgur] remove unnecessary code and add object check

* remove spec_name from validation code

* Remove unnecessary code and add owners file

* Change required tag name  to  in validation file

* data-imgurid -> data-imgur-id

* Add whitelist for amp-imgur

* remove unused AmpImgur import in test file

* remove unused preconnect callback

* Update amp-cors-requests.md (ampproject#9636)

Added updates and corrections per Dima's feedback.

* Expose login url building to amp access service (ampproject#9300)

* Animations: support style keyframes (ampproject#9647)

* Animations: support style keyframes

* docs

* review fixes

* lints

* Animations: subtargets format (ampproject#9655)

* Animations: subtargets format

* fix docs

* Fix extern for integration test (ampproject#9657)

* Remove whitelisted link for PR 9434 (ampproject#9656)

* Add callouts for CORS + cleanup (ampproject#9591)

* Add callouts for CORS + cleanup

* Update amp-access.md

* Doubleclick Fast Fetch: Send default safeframe version on ad request (ampproject#9613)

* Send default safeframe version on ad request

* fix test failure

* amp-ima-video: Fixes some undefined variables in compiled extension (ampproject#9617)

* Add experimental input-debounced to the actions and event doc (ampproject#9650)

* Copy and rename compiled script directly for deprecated version (ampproject#9587)

* Remove A4A Dependency on ads/_config.js (ampproject#9462)

* Added doubleclick specific config

* Modified other networks

* Update MockA4AImpl

* Remove unneeded config lines

* Add cid, remove renderStartImplemented

* Fix lint complaint

* Changed from mandatory config to overrrideable method

* Addressed feedback, modified getAdCid

* Addressed additional feedback

* Responded to feedback

* Removed unnneeded guard against null value

* Changed signature of function

* Fixed lint issues

* addressed comments

* Addressed comments, fixed broken test

* Change signature / update tests

* Fix wrong contents and white spaces in amp-imgur.md (ampproject#9658)

* Performance: separate ini-load from first visible ini-load (ampproject#9665)

* amp-bind: Fix more integration tests (ampproject#9598)

* more bind test fixes

* include object.assign polyfill

* move polyfills to separate file

* replace use of map() from bind-validator with ownProperty()

* fix types and test

* Stop using IOS elastic scroll when slidescroll takes over animation (Only for the NON SNAP-POINTS flow) (ampproject#9668)

* removing overflow touch on no-scroll for ios

* Stop using IOS elastic scroll when slidescroll takes over animation (Only for the NON SNAP-POINTS flow)

* make sure css is compiled before entry points (ampproject#9643)

* update npm package version (ampproject#9671)

* A4A: expose visibilityCsi (ampproject#9667)

* A4A: expose visibilityCsi

* tests

* amp-bind: Fix embedding in FIF (ampproject#9541)

* move element service changes from ampproject#9447

* store element service ids in extension struct

* add unit test, fix other tests

* Fix null value binding (ampproject#9674)

* Add amp-form's submit action to the docs (ampproject#9675)

* Write cookie for ad-cid. (ampproject#9594)

* Write cookie for ad-cid.

* fix tests

* amp-access-laterpay fixes (ampproject#9633)

* Add support for LaterPay subscriptions

* Fix canonical link on laterpay example

* Tweak default styling

* Add an example locale message for the header

* Fix indentation of JsDoc (ampproject#9677)

It should be indented at the same level as the method it's documenting.

* Add example to amp iframe docs (ampproject#9660)

* Change image to static link hosted on ampproject.org

* Update amp-iframe doc w example + gen cleanup

* Adds SFG experiment branches to doubleclick-a4a-config.js. (ampproject#9662)

* Added experiment branches corresponding to exp=a4a:(5|6).

* Removed test file.

* Removed changes to yarn.lock + minor fixes.

* Fixed adsense-a4a-config.js.

* Fixed return statement.

* Removed reference to style tag for opacity (ampproject#9634)

As style tag for `opacity` was replaced with `amp-boilerplate (back in this [commit](ampproject@0a056ca), updated the text to reflect those changes.

* De-flake amp-bind integration tests (ampproject#9683)

* split bind fixtures into one file per extension

* fix width/height binding bug, avoid race condition w/ dynamic containers

* actually, just skip the tests affected by race condition for now

* Remove timer calls to prevent future flakiness (ampproject#9478)

* Remove timer calls to prevent future flakiness

* Revert the line order change

* Poll instead of using stub callback constructor

* Add jsdoc and rename variables

* amp-animation: polyfill partial keyframes from CSS (ampproject#9689)

* Animations: whitelist offset distance and regroup condition types (ampproject#9688)

* Use Docker containers in Travis (ampproject#9666)

* clean up web-worker experiment (ampproject#9706)

* Load examiner.js when #development=2 (ampproject#9680)

* Load examiner.js when #development=1

* address comments

* Use development=2

* Fix presubmit

* Implementation of amp-ad-exit (ampproject#9390)

* Implementation of the amp-ad-exit component for managing ad navigation (ampproject#8515).

Changes from the I2I:
- Only RANDOM, CLICK_X, and CLICK_Y variables can be used, alongside custom vars.
- It doesn't work well with <a> tags - the tap action doesn't trigger on middle clicks. Recommendation is to use other elements.
- ClickLocationFilter for border click protection is not yet implemented. It will be added in a future change.

The component does some lightweight validation of the JSON config when it's
built. This may be overkill.

Tested:
gulp test --files 'extensions/amp-ad-exit/0.1/test/*'
gulp check-types

* Update JSDoc for some functions. Fix lint issue (ignore unused parameters in an interface method).

* sinon.spy -> sandbox.spy

* Add amp-ad-exit to the list of allowed extensions in A4A pages and fix validation rules.

Address PR: add experiment flag; use camelCase for config properties

address some comments

Instantiate a new Filter for each spec.

* Instantiate a new Filter for each spec.

* remove duplicate doctype tag in example

* Make ActionEventDef public for type annotations.

* Add filter factory.

* fix event var

* update type annotation for Filter.filter

* address PR comments

* Move filter config validation to the ctor. Remove Filter.buildCallback().

* camelCase in documentation examples

* Introducing "it.yield", a convenient way to test promise code (ampproject#9601)

* Introduce yield.

* Address comments.

* fix done state error handling

* Document amp-access as a special target (ampproject#9568)

`amp-access` is a special target that's not mentioned in this document.
It's special because you can't give an arbitrary ID to it, i.e., you
can't just change the id of the `amp-access` script tag to `amp-access2`
and expect `on="tap:amp-access2.login"` to work. Given that the
"actions" for `amp-access` is a bit dynamic depending on the structure
of the `amp-access` JSON, instead of listing out the actions in a
tabular form, a reference to the `amp-access` documentation is added.

* Run presubmit tests soon after building (ampproject#9716)

* Revert "Use Docker containers in Travis (ampproject#9666)" (ampproject#9717)

This reverts commit 98ecb9b.

It turns out that while using Docker containers instead of VMs on Travis reduces VM startup time, it has significantly increased build and test time, increasing the overall PR check round trip from < 20 mins to > 30 mins.

Compare https://travis-ci.org/ampproject/amphtml/jobs/238464404 (Docker) against https://travis-ci.org/ampproject/amphtml/jobs/238462066 (VM).

Fixes ampproject#9651

* Validator Rollup (ampproject#9719)

* Add validator tests for amp-imgur.

* Disallow amp-embed as child of amp-app-banner.

* Minor cleanup, Make sure our set numbers are consistent in javascript.

* amp-state: Allow both `src` and script child (ampproject#9721)

* adapt code from ampproject#8897

* also_requires_attr should be in trigger

* Remove experiment for input-debounced. Update docs. (ampproject#9724)

* Validator Rollup (ampproject#9727)

* Minor cleanup of amp-ad-exit

* Disallow amp-ad/amp-embed as children of amp ad containers when data-multi-size is present.

* Revision bump

* Enabling dynamic queryparam addition to anchour links  (ampproject#9684)

* Enabling queryparam addition to anchour links

* Code review changes

* Additional cache urls (ampproject#9733)

* Significantly speed up gulp build --css-only (ampproject#9726)

Prior to this PR, gulp build --css-only would take ~ 1 minute to run, and end up building a bunch of js files in addition to compiling css.

After this PR, gulp build --css-only takes ~ 0.5 seconds to run (a 100x speed up), and the only output files in build/ after running it are .css, and .css.js files.

Resulting speeds on my workstation:
gulp build --css-only: ~500ms
gulp build: ~1m 45s
gulp dist --fortesting: ~3m 30s

Fixes ampproject#9640

* add amp-analytics Facebook Pixel support (ampproject#9449)

* add amp-analytics Facebook Pixel support

* add amp-analytics Facebook Pixel support

* fix test on extensions/amp-analytics/0.1/test/vendor-requests.json

* add Facebook Pixel standard events https://developers.facebook.com/docs/ads-for-websites/pixel-events/v2.9#events

* Update vendors.js

fix conflicts

* Update vendors.js

fix conflicts

* Update vendor-requests.json

fix conflicts

* Update vendor-requests.json

fix conflicts

* Update vendor-requests.json

* Update analytics.amp.html

* Update analytics-vendors.amp.html

* Update vendors.js

* Update analytics-vendors.amp.html

* fix lint error

* update

* facebook pixel

* Update vendor-requests.json

* Update analytics.amp.html

* fix lint error

* fix lint error

*  fix alphabetical order

* Update yarn.lock

* AMP-ad supports Seznam Imedia ad network (ampproject#8893)

* AMP-ad supports Seznam Imedia ad network

* Fixed requested changes

* Fixed requested changes

* Render API implemented

* Improved placing ads

* Fixed requested changes

* Render API fixed context
* Renamed IM.cz to Imedia
* Described JSON parameters
@honeybadgerdontcare
Copy link
Contributor

This is now live in prod.

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.

None yet

6 participants