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

Add URL rewriting and srcset support to amp-bind #7206

Merged

Conversation

dreamofabear
Copy link

Partial for #6199.

  • Rewrite image URLs on CDN-served AMP pages
  • Support srcset attribute in BindValidator

@dreamofabear
Copy link
Author

/to @aghassemi /cc @dvoytenko

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.

Look good, just a small change request.

@@ -192,7 +250,9 @@ function createElementRules_() {
},
blockedURLs: ['__amp_source_origin'],
},
// TODO: Support `srcset`.
srcset: {
Copy link
Contributor

Choose a reason for hiding this comment

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

there is no srcset on amp-video

Copy link
Author

Choose a reason for hiding this comment

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

Woops, done.

@@ -211,6 +227,24 @@ describe('BindValidator', () => {
/* eslint no-script-url: 0 */ 'javascript:alert(1)\n;')).to.be.false;
expect(val.isResultValid(
'AMP-VIDEO', 'src', '__amp_source_origin')).to.be.false;

// srcset
expect(val.isResultValid(
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto

Copy link
Author

Choose a reason for hiding this comment

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

Done.

Copy link
Author

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

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

Thanks for the review.

@@ -192,7 +250,9 @@ function createElementRules_() {
},
blockedURLs: ['__amp_source_origin'],
},
// TODO: Support `srcset`.
srcset: {
Copy link
Author

Choose a reason for hiding this comment

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

Woops, done.

@@ -211,6 +227,24 @@ describe('BindValidator', () => {
/* eslint no-script-url: 0 */ 'javascript:alert(1)\n;')).to.be.false;
expect(val.isResultValid(
'AMP-VIDEO', 'src', '__amp_source_origin')).to.be.false;

// srcset
expect(val.isResultValid(
Copy link
Author

Choose a reason for hiding this comment

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

Done.

@dreamofabear dreamofabear merged commit 4ecfb21 into ampproject:master Jan 27, 2017
@dreamofabear dreamofabear deleted the bind-url-rewrite-and-srcset branch January 27, 2017 20:05
jridgewell pushed a commit to jridgewell/amphtml that referenced this pull request Jan 31, 2017
* add srcset support

* fix types

* rewrite urls for cache

* ignore compiler warnings in third_party/caja

* whitelist amp-bind to depend on sanitizer.js

* ali's comments
torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Feb 14, 2017
* add srcset support

* fix types

* rewrite urls for cache

* ignore compiler warnings in third_party/caja

* whitelist amp-bind to depend on sanitizer.js

* ali's comments
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
* add srcset support

* fix types

* rewrite urls for cache

* ignore compiler warnings in third_party/caja

* whitelist amp-bind to depend on sanitizer.js

* ali's comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants