Unload resources in carousel #2123
Conversation
erwinmombay
force-pushed
the
unload-video
branch
3 times, most recently
from
d75a1fd
to
7eda331
Compare
|
@dvoytenko mind giving this a first, pass. super preliminary work. just want to make sure im in the right direction. |
erwinmombay
force-pushed
the
unload-video
branch
2 times, most recently
from
3386f97
to
ae9f721
Compare
| // when you null out an img tags src and you read it, it will be | ||
| // window.location.origin + window.location.path so we need | ||
| // to compare to the previously selected "src" above. | ||
| if (this.img_.src == '' || this.img_.src != src) { |
There was a problem hiding this comment.
You can use img.getAttribute('src') instead.
img.src = '';
img.src; // => `${location.origin}${location.path}`
img.getAttribute('src'); // => ''
erwinmombay
force-pushed
the
unload-video
branch
3 times, most recently
from
7ca7d81
to
33d0b69
Compare
| this.loadPromise_ = loadPromise(this.img_); | ||
| this.loadPromise_ = loadPromise(this.img_) | ||
| .catch(error => { | ||
| if (this.img_.getAttribute('src')) { |
There was a problem hiding this comment.
You mean !this.img_.getAttribute('src'), right? If src was cleared, means that we expect the error.
There was a problem hiding this comment.
yep, sorry just made the change hastily here and just havent pushed out latest changes with tests.
will mark done when ive pushed.
|
Made few comments. Generally, it's very close. Please add tests. |
erwinmombay
force-pushed
the
unload-video
branch
3 times, most recently
from
947fc82
to
12406a6
Compare
|
@dvoytenko please review. should be finalized now. i still need to add more tests, but everything on amp-carousel integration tests are failing for me locally currently...trying to figure that out. |
erwinmombay
changed the title
erwinmombay
force-pushed
the
unload-video
branch
from
12406a6
to
42b9d6d
Compare
|
@dvoytenko friendly ping. would like to wrap this up by EOB (yes eob!) |
|
|
||
| describe('integration amp-carousel', () => { | ||
|
|
||
| let fixture; | ||
| //let sandbox; |
There was a problem hiding this comment.
haha yeah, meant to leave it in intentionally. as my tests are all red locally for carousels. will remove and move it over to a different branch.
|
LGTM |
erwinmombay
force-pushed
the
unload-video
branch
from
42b9d6d
to
4156069
Compare
erwinmombay
force-pushed
the
unload-video
branch
from
4156069
to
ba33ca2
Compare
Unload resources in carousel
closes #1293
closes #1334