Allow any layout for amp-social-share, with test. #3099
Conversation
|
LGTM |
| @@ -28,6 +28,7 @@ | |||
| <script async src="https://cdn.ampproject.org/v0.js"></script> | |||
| </head> | |||
| <body> | |||
| <!-- a fixed-layout example (layout is implicitly fixed) --> | |||
| <amp-social-share type="pinterest" width="60" height="44"> | |||
| <script type="application/json"> | |||
There was a problem hiding this comment.
We dropped the script configuration support can we invalidate that?
- Any type is allowed, it's just a string. - data-share-endpoint is optional. If given, it must be a URL with one of the whitelisted protocols. Relative URLs are not allowed.
|
@mkhatib I updated this, as discussed. Please take another look! Thanks. |
| <!-- Valid: unknown type, no endpoint given | ||
| Note: It would be good to consider this invalid, but given the choices | ||
| around required / optional parameters for this tag, we don't have a | ||
| convenient way to do that. This will probablly yield a runtime error. |
There was a problem hiding this comment.
Yes. I think runtime error should be good enough. @dvoytenko what do you think?
There was a problem hiding this comment.
Yes. I believe this should be a runtime error, not a validator error.
|
Thanks @powdercloud! un-LGTM for now to get @dvoytenko's input on some of the points we chatted about. |
|
Filed #3165 to do some research on other popular app protocols. But for now this LGTM. Thanks for getting this in! One this is in production validator we can probably turn off experiment on amp-social-share - there are few publishers asking for it. |
|
pinging @dvoytenko for a final look. |
|
LGTM from me as well. |
Fixes #3014