Refactor getElementPriority #3120
Conversation
muxin
force-pushed
the
refactor-getElementPriority
branch
from
77098a8
to
83604fb
Compare
| @@ -109,6 +109,7 @@ import {vsyncFor} from './vsync'; | |||
| * Each method is called exactly once and overriding them in subclasses | |||
| * is optional. | |||
| */ | |||
|
|
|||
| @@ -49,6 +49,11 @@ export function installAd(win) { | |||
|
|
|||
| class AmpAd extends BaseElement { | |||
|
|
|||
| /** @override */ | |||
| getElementPriority() { | |||
| return 2; | |||
There was a problem hiding this comment.
Please add something like
// loads ads after other content and analytics.
|
Excellent, thanks! LGTM from me. Passing on to @mkhatib for final LGTM. |
| @@ -27,6 +27,11 @@ export class ElementStub extends BaseElement { | |||
| } | |||
|
|
|||
| /** @override */ | |||
| getPriority() { | |||
| throw new Error('Cannot get priority of stubbed element'); | |||
There was a problem hiding this comment.
@dvoytenko is it ok to just through error like this here? No need to dev.assert it?
There was a problem hiding this comment.
This is fine. Both do very similar things and both we want to show up in our production error reporting, because they are not expected to actually happen.
There was a problem hiding this comment.
@cramforce not quite dev.assert is DCE'd so it won't be in prod errors reporting. But, I agree, let's leave new Error() here so that we actually do get the error.
|
LGTM! Please rebase and wait for the tests to pass. |
|
LGTM |
muxin
force-pushed
the
refactor-getElementPriority
branch
from
cb3aa3f
to
9aa5f43
Compare
|
💃 💃 💃 💃 💃 💃 |
|
First commit? 🎆 🎊 👏 |
fixes #2714