Making amp-video an extension + backwards compatibility (the real one!) (used git mv here)#6686
Making amp-video an extension + backwards compatibility (the real one!) (used git mv here)#6686chenshay wants to merge 10 commits intoampproject:masterfrom
Conversation
|
Could you run Thanks! |
cramforce
left a comment
There was a problem hiding this comment.
Nice. This was simpler than I thought :) See comment above about assessing size impact.
I think @aghassemi figured out a way for adding a warnings that sites can use to add the script tag.
| import * as sinon from 'sinon'; | ||
|
|
||
| describe('amp-video', () => { | ||
| describe('amp-video, backwards compatibility', () => { |
There was a problem hiding this comment.
Can/should we add any assertions here?
|
@chenshay The tool actually generated a file that you can commit into the change to see a diff. Or just use |
aghassemi
left a comment
There was a problem hiding this comment.
This is looking great! Just a few more requests.
Files still are not showing up as a move but that's okay, we can leave with that history is not lost anyway, there are ways to get to it.
| /** @private {?Function} */ | ||
| this.playerReadyResolver_ = null; | ||
|
|
||
| const ampdoc = ampdocServiceFor(this.win).getAmpDoc(); |
There was a problem hiding this comment.
let's move this to buildCallback and keep constructor purely for declarations
| /** @private {?Element} */ | ||
| this.video_ = null; | ||
|
|
||
| const ampdoc = ampdocServiceFor(this.win).getAmpDoc(); |
There was a problem hiding this comment.
ditto, move to buildCallback
| # See the License for the specific language governing permissions and | ||
| # limitations under the license. | ||
| # | ||
|
|
There was a problem hiding this comment.
we still need the "script" section you had in the previous PR. with unique_warning: true changed to unique: true as @honeybadgerdontcare suggested.
(The script section is about validation rules on the <script src=..amp-video-0.1-> itself, something that did no exist before but now is needed since amp-video is its own extension now)
| /** @private {?Function} */ | ||
| this.playerReadyResolver_ = null; | ||
|
|
||
| const ampdoc = ampdocServiceFor(this.win).getAmpDoc(); |
| attrs: { name: "height" } | ||
| attrs: { name: "width" } | ||
| } | ||
| # 4.7.6 The video element |
There was a problem hiding this comment.
don't remove this section. This one is about <video> not <amp-video>
| html_format: AMP4ADS | ||
| tag_name: "AMP-VIDEO" | ||
| disallowed_ancestor: "AMP-SIDEBAR" | ||
| also_requires_tag_warning: "amp-video extension .js script" |
There was a problem hiding this comment.
@cramforce This rule is the one that creates the warning if users don't include the <script> tag.
There was a problem hiding this comment.
validation lgtm
|
@chenshay For the failing tests: |
| tag_name: "AMP-VIDEO" | ||
| spec_name: "amp-video extension .js script" | ||
| mandatory_parent: "HEAD" | ||
| unique_warning: true |
There was a problem hiding this comment.
This needs to be unique: true. It shouldn't be a warning if it is new.
There was a problem hiding this comment.
Please move unique, mandatory_parent and spec_name to SCRIPT instead of AMP-VIDEO. Add also_requires_tag: amp-video to SCRIPT.
validator/validator-main.protoascii
Outdated
| } | ||
| attrs: { name: "width" } | ||
| spec_url: "https://www.ampproject.org/docs/reference/amp-video.html" | ||
| } |
There was a problem hiding this comment.
Looks like erroneous whitespace and tabs got introduced here. Please fix.
|
Note that #6680 made changes to old amp-video, needs to be merged with this one |
|
tried git mv another time: |
Uh oh!
There was an error while loading. Please reload this page.