Use visibility to show/hide next slides to allow preloading to actually work #2913
Conversation
|
cc/ @dvoytenko |
|
update the tests? |
|
I couldn't find any tests that actually test for preloads or visibility, I can add some test cases though |
|
yes please! ping me once you have added the tests. |
mkhatib
force-pushed
the
actually-preload-slides
branch
from
620521e
to
74c6cdb
Compare
|
@sriramkrish85 added test cases PTAL |
mkhatib
force-pushed
the
actually-preload-slides
branch
from
74c6cdb
to
89aacc8
Compare
|
@cramforce - do you wanna take a final pass? |
| it('should preload slides in direction of navigation', () => { | ||
| expect(slide0.style.visibility).to.be.equal('visible'); | ||
| expect(slide1.style.visibility).to.be.equal('hidden'); | ||
| expect(slides.scheduleLayout.calledWith(slide0)); |
There was a problem hiding this comment.
Actually I think these are not running properly. These need to .to.be.true appended, will update.
There was a problem hiding this comment.
Fixed should now actually run these.
mkhatib
force-pushed
the
actually-preload-slides
branch
from
7c9c40e
to
00073dc
Compare
| @@ -49,7 +49,7 @@ export class AmpSlides extends BaseCarousel { | |||
| this.slides_.forEach((slide, i) => { | |||
| this.setAsOwner(slide); | |||
| // Only the first element is initially visible. | |||
| slide.style.display = i > 0 ? 'none' : 'block'; | |||
There was a problem hiding this comment.
I just need some context here: display: none is much better from a performance pov. But if we do want to render them, then this change is good.
There was a problem hiding this comment.
Yeah the problem with display: none is that resources will totally ignore the element and won't schedule preload for it although the carousel is asking the resources to schedule preload for them. Some more context on #2742
If we expect a big hit on performance maybe a compromise is to use both visibility and display. Where visibility would be used for for +1/-1 window length (e.g. for slider this is 1 slide before and 1 slide after) and display:none the rest outside that window. This would complicate the logic slightly but if we expect the performance to be worth it maybe it's worth getting it right.
In anyway, since we might be working on big changes for carousel maybe we can keep that in mind in the new design.
There was a problem hiding this comment.
+1 Make sure to comment that there when we are reviewing the design.
mkhatib
force-pushed
the
actually-preload-slides
branch
from
00073dc
to
5972e2e
Compare
…reloading to actually work.
mkhatib
force-pushed
the
actually-preload-slides
branch
from
5972e2e
to
6b999ad
Compare
Fixes #2742