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

Update amp-bind.md #7704

Merged
merged 3 commits into from Feb 24, 2017
Merged

Update amp-bind.md #7704

merged 3 commits into from Feb 24, 2017

Conversation

kmh287
Copy link
Contributor

@kmh287 kmh287 commented Feb 21, 2017

Add width and height as attributes that support bindings on amp-img

@@ -98,7 +98,7 @@ For AMP components, only select attributes are bindable:
| Component | Attributes |
| --- | --- |
| amp-carousel | slide |
| amp-img | src, srcset, alt |
| amp-img | src, srcset, alt, height, width |

Choose a reason for hiding this comment

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

We actually support [width] and [height] on all AMP extensions. Can you remove this from the table and maybe add a sentence above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then should this include the other three supported tags?

Copy link

Choose a reason for hiding this comment

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

@choumx - Are the common attributes for AMP components bindable? If so, then as you say, remove height and width and replace this sentence:

"For AMP components, only select attributes are bindable"

with this:

"For AMP components, the common attributes are bindable along with the following specific attributes:"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@choumx can correct me if I'm wrong but I don't think we're going to support all of those attributes. For instance the on attribute is used to communicate state changes to bind; allowing that itself to be a bind attribute could get pretty hairy.

Choose a reason for hiding this comment

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

Correct, only width and height for all AMP extensions for now. Looks like the implementation in bind-validator.js is out of date.

Copy link

Choose a reason for hiding this comment

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

Okay, then I suggest rewriting that sentence to:

For AMP components, the height and width attributes are bindable along with the following specific attributes:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks!

@jridgewell jridgewell requested a review from a user February 23, 2017 01:43
@kmh287
Copy link
Contributor Author

kmh287 commented Feb 24, 2017

@choumx please merge

@dreamofabear dreamofabear merged commit 16a1d47 into master Feb 24, 2017
@mrjoro mrjoro deleted the kmh287-amp-bind-md-update branch April 10, 2017 20:06
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
* Update amp-bind.md

* Update amp-bind.md

* Update amp-bind.md
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.

None yet

2 participants