AMP jwplayer #2709
AMP jwplayer #2709
Conversation
|
@rudygalfi @dvoytenko Can we add the "needs review" tag to this PR? |
|
|
||
| /** @override */ | ||
| layoutCallback() { | ||
| /** @private @const {string} */ |
There was a problem hiding this comment.
Could you actually move this.contentid_ and this.playerid_ declarations into the buildCallback - it's much better to report these errors as soon as possible.
There was a problem hiding this comment.
@dvoytenko I had done that, but when the assertions are in buildCallback, I get validator errors for the tests:
AssertionError: expected promise to be rejected with an error matching /Either the data-media-id
or the data-playlist-id attributes must be/ but it was fulfilled with <amp-jwplayer class="-amp-
element -amp-notbuilt amp-notbuilt -amp-layout-responsive -amp-layout-size-defined -amp-
error" data-player-id="sDZEo0ea" width="320" height="180" layout="responsive" id="AMP_1">
<i-amp-sizer style="display: block; padding-top: 56.25%;"></i-amp-sizer><iframe
frameborder="0" allowfullscreen="true" src="https://content.jwplatform.com/players/undefined-
undefined.html" class="-amp-fill-content" width="320" height="180"></iframe></amp-jwplayer>
Should I add an additional assertion in layoutCallback? Or is there an issue in the tests?
There was a problem hiding this comment.
I see. I'd rather move them to buildCallback and lose that test, than delay errors. So, let's move the asserts and remove affected test (it looks like it's just the one).
There was a problem hiding this comment.
@dvoytenko OK, I removed the tests. There were two of them, both related to the same thing. Squashing the commits now.
|
@pablos1 All looks good. Just two small requests to improve the error messaging and timing. Once done, could you please also squash the commits? |
|
@dvoytenko One quick question on the error timing. Once that looks good, I'll squash the commits. |
|
LGTM. Merging. Thanks a lot! |
|
Thanks! |
|
Thanks, everyone! @dvoytenko , will this component be rolled into the next release? JL |
|
@johnluther this will get released to canary this thursday, full production next week thursday |
|
Awesome, thanks @erwinmombay ! JL |
Implements #2125 (continues #2660)