Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use visibility to show/hide next slides to allow preloading to actually work #2913

Merged
merged 3 commits into from
Apr 19, 2016
Merged

Use visibility to show/hide next slides to allow preloading to actually work #2913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
05ce3a4
Use visibility instead of display to show/hide next slides to allow p…
mkhatib Apr 14, 2016
6b999ad
Actually run the expectations.
mkhatib Apr 18, 2016
e2c8ba3
Fix linting
mkhatib Apr 19, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 5 additions & 5 deletions extensions/amp-carousel/0.1/slides.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
slide.style.visibility = i > 0 ? 'hidden' : 'visible';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 Make sure to comment that there when we are reviewing the design.

this.applyFillContent(slide);
});

Expand Down Expand Up @@ -177,7 +177,7 @@ export class AmpSlides extends BaseCarousel {
st.setStyles(slide, {
transform: st.translateX(dir * containerWidth),
zIndex: 1,
display: 'block',
visibility: 'visible',
});

this.scheduleLayout(slide);
Expand All @@ -197,7 +197,7 @@ export class AmpSlides extends BaseCarousel {
});
} else {
st.setStyles(slide, {
display: 'none',
visibility: 'hidden',
zIndex: 0,
transform: '',
opacity: 1,
Expand Down Expand Up @@ -232,13 +232,13 @@ export class AmpSlides extends BaseCarousel {
*/
commitSwitch_(oldSlide, newSlide) {
st.setStyles(oldSlide, {
display: 'none',
visibility: 'hidden',
zIndex: 0,
transform: '',
opacity: 1,
});
st.setStyles(newSlide, {
display: 'block',
visibility: 'visible',
zIndex: 0,
transform: '',
opacity: 1,
Expand Down
70 changes: 69 additions & 1 deletion extensions/amp-carousel/0.1/test/test-slides.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import * as sinon from 'sinon';
import * as tr from '../../../../src/transition';
import {AmpSlides} from '../slides';

import {Animation} from '../../../../src/animation';

describe('Slides functional', () => {

Expand Down Expand Up @@ -771,6 +771,12 @@ describe('Slides functional', () => {
slides.updateInViewport = sandbox.spy();
slides.schedulePause = sandbox.spy();
slides.schedulePreload = sandbox.spy();

Animation.animate = () => {
return {
thenAlways: cb => cb(),
};
};
});
afterEach(() => {
sandbox.restore();
Expand All @@ -791,6 +797,68 @@ describe('Slides functional', () => {
expect(slides.updateInViewport.calledWith(slide1, false)).to.be.true;
});
});

describe('Navigating slides', () => {
beforeEach(() => {
sandbox = sinon.sandbox.create();
setupElements();
setupSlides();
slides.inViewport_ = true;
slides.getVsync = function() {
return {
mutate: function(fn) {
fn();
},
};
};
slides.deferMutate = function(fn) {
fn();
};
slides.scheduleLayout = sandbox.spy();
slides.updateInViewport = sandbox.spy();
slides.schedulePause = sandbox.spy();
slides.schedulePreload = sandbox.spy();
});
afterEach(() => {
sandbox.restore();
teardownElements();
});

it('should hide slides that are not the current one', () => {
expect(slide0.style.visibility).to.be.equal('visible');
expect(slide1.style.visibility).to.be.equal('hidden');
expect(slide2.style.visibility).to.be.equal('hidden');

slides.goCallback(1, /*animate*/ false);
expect(slide0.style.visibility).to.be.equal('hidden');
expect(slide1.style.visibility).to.be.equal('visible');
expect(slide2.style.visibility).to.be.equal('hidden');

slides.goCallback(-1, /*animate*/ false);
expect(slide0.style.visibility).to.be.equal('visible');
expect(slide1.style.visibility).to.be.equal('hidden');
expect(slide2.style.visibility).to.be.equal('hidden');
});

it('should preload slides in direction of navigation', () => {
expect(slide0.style.visibility).to.be.equal('visible');
expect(slide1.style.visibility).to.be.equal('hidden');

slides.goCallback(1, /*animate*/ true);
expect(slide0.style.visibility).to.be.equal('hidden');
expect(slide1.style.visibility).to.be.equal('visible');
expect(slide2.style.visibility).to.be.equal('hidden');
expect(slides.scheduleLayout.calledWith(slide1)).to.be.true;
expect(slides.schedulePreload.calledWith(slide2)).to.be.true;

slides.goCallback(-1, /*animate*/ false);
expect(slide0.style.visibility).to.be.equal('visible');
expect(slide1.style.visibility).to.be.equal('hidden');
expect(slide2.style.visibility).to.be.equal('hidden');
expect(slides.scheduleLayout.calledWith(slide0)).to.be.true;
expect(slides.schedulePreload.calledWith(slide2)).to.be.true;
});
});
});

describe('empty Slides functional', () => {
Expand Down