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.

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

Amp slidescroll experiment - Handling Native Snapping #3798

Merged
merged 9 commits into from
Jun 28, 2016
4 changes: 4 additions & 0 deletions extensions/amp-carousel/0.1/amp-carousel.css
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,10 @@ amp-carousel .amp-carousel-button.amp-disabled {
-webkit-overflow-scrolling: touch !important;
}

.-amp-slides-container.no-scroll {
overflow-x: hidden !important;
}

.-amp-slide-item {
align-items: center !important;
display: none !important;
Expand Down
109 changes: 97 additions & 12 deletions extensions/amp-carousel/0.1/slidescroll.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import {BaseCarousel} from './base-carousel';
import {Layout} from '../../../src/layout';
import {getStyle, setStyle} from '../../../src/style';
import {timer} from '../../../src/timer';

/** @const {string} */
const SHOWN_CSS_CLASS = '-amp-slide-item-show';
Expand All @@ -30,6 +31,9 @@ export class AmpSlideScroll extends BaseCarousel {
/** @private @const {!Window} */
this.win_ = this.getWin();

/** @const @private {!Vsync} */
this.vsync_ = this.getVsync();

/** @private @const {!boolean} */
this.hasNativeSnapPoints_ = (
getStyle(this.element, 'scrollSnapType') != undefined);
Expand All @@ -38,6 +42,13 @@ export class AmpSlideScroll extends BaseCarousel {
/** @private {!Array<!Element>} */
this.slides_ = this.getRealChildren();

/** @private {number} */
this.noOfSlides_ = this.slides_.length;

/** @private @const {boolean} */
this.hasLooping_ =
Copy link
Member

Choose a reason for hiding this comment

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

This is already assigned to above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

this.element.hasAttribute('loop') && this.noOfSlides_ > 1;

/** @private {!Element} */
this.slidesContainer_ = this.win_.document.createElement('div');
this.slidesContainer_.classList.add('-amp-slides-container');
Expand Down Expand Up @@ -67,12 +78,14 @@ export class AmpSlideScroll extends BaseCarousel {

this.element.appendChild(this.slidesContainer_);

/** @private {number} */
this.noOfSlides_ = this.slides_.length;

/** @private @const {boolean} */
this.hasLooping_ =
this.element.hasAttribute('loop') && this.noOfSlides_ > 1;
this.snappingInProgress_ = false;

/** @private {?number}*/
this.scrollTimeout_ = null;

this.slidesContainer_.addEventListener(
'scroll', this.scrollHandler_.bind(this));
}

/** @override */
Expand All @@ -99,10 +112,6 @@ export class AmpSlideScroll extends BaseCarousel {
}
}

/** @override */
setupGestures() {
}

/** @override */
hasPrev() {
return this.hasLooping_ || this.slideIndex_ > 0;
Expand All @@ -129,6 +138,82 @@ export class AmpSlideScroll extends BaseCarousel {
}
}

/**
* Handles scroll on the slides container.
* @param {!Event} unusedEvent Event object.
* @private
*/
scrollHandler_(unusedEvent) {
if (this.scrollTimeout_) {
timer.cancel(this.scrollTimeout_);
}
const currentScrollLeft = this.slidesContainer_./*OK*/scrollLeft;
if (currentScrollLeft != this.previousScrollLeft_ &&
!this.hasNativeSnapPoints_) {
// TODO(sriramkrish85): Handle custom scroll here.
}

// Timer that detects scroll end and/or end of snap scroll.
this.scrollTimeout_ = timer.delay(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Add some docs as to why this delay.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done - let me know if that looks okay.

if (this.snappingInProgress_) {
return;
}
if (this.hasNativeSnapPoints_) {
this.updateOnScroll_(currentScrollLeft);
}
}, 100);
this.previousScrollLeft_ = currentScrollLeft;
}


/**
* Updates to the right state of the new index on scroll.
* @param {number} currentScrollLeft scrollLeft value of the slides container.
*/
updateOnScroll_(currentScrollLeft) {
this.snappingInProgress_ = true;
// This can be only 0, 1 or 2, since only a max of 3 slides are shown at
// a time.
const scrolledSlideIndex = Math.round(currentScrollLeft / this.slideWidth_);
// Update value can be -1, 0 or 1 depending upon the index of the current
// shown slide.
let updateValue = 0;

const hasPrev_ = this.hasPrev();
const hasNext_ = this.hasNext();

if (hasPrev_ && hasNext_) {
updateValue = scrolledSlideIndex - 1;
} else if (hasNext_) {
// Has next and does not have a prev. (slideIndex 0)
updateValue = scrolledSlideIndex;
} else if (hasPrev_) {
// Has prev and no next slide (last slide)
updateValue = scrolledSlideIndex - 1;
}

let newIndex = this.slideIndex_ + updateValue;

if (this.hasLooping_) {
newIndex = (newIndex < 0) ? this.noOfSlides_ - 1 :
(newIndex >= this.noOfSlides_) ? 0 : newIndex;
} else {
newIndex = (newIndex < 0) ? 0 :
(newIndex >= this.noOfSlides_) ? this.noOfSlides_ - 1 : newIndex;
}
this.vsync_.mutate(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Document this nested mutate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

// Make the container non scrollable to stop scroll events.
this.slidesContainer_.classList.add('no-scroll');
// Scroll to new slide and update scrollLeft to the correct slide.
this.showSlide_(newIndex);
this.vsync_.mutate(() => {
// Make the container scrollable again to enable user swiping.
this.slidesContainer_.classList.remove('no-scroll');
this.snappingInProgress_ = false;
});
});
}

/**
* Makes the slide corresponding to the given index and the slides surrounding
* it available for display.
Expand All @@ -138,11 +223,10 @@ export class AmpSlideScroll extends BaseCarousel {
showSlide_(newIndex) {
const noOfSlides_ = this.noOfSlides_;
if (newIndex < 0 ||
newIndex >= noOfSlides_ ||
this.slideIndex_ == newIndex) {
newIndex >= noOfSlides_ ||
this.slideIndex_ == newIndex) {
return;
}

const prevIndex = (newIndex - 1 >= 0) ? newIndex - 1 :
(this.hasLooping_) ? noOfSlides_ - 1 : null;
const nextIndex = (newIndex + 1 < noOfSlides_) ? newIndex + 1 :
Expand Down Expand Up @@ -179,6 +263,7 @@ export class AmpSlideScroll extends BaseCarousel {
if (!this.hasLooping_ && newIndex == 0) {
newScrollLeft = 0;
}

this.slidesContainer_./*OK*/scrollLeft = newScrollLeft;
this.slideIndex_ = newIndex;
this.hideRestOfTheSlides_(showIndexArr);
Expand Down
93 changes: 92 additions & 1 deletion extensions/amp-carousel/0.1/test/test-slidescroll.js
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,6 @@ describe('SlideScroll', () => {
impl.showSlide_(0);

expect(hideRestOfTheSlidesSpy).to.have.been.calledWith([0,1]);

expect(impl.slideWrappers_[0].classList.contains(SHOW_CLASS))
.to.be.true;
expect(impl.slideWrappers_[1].classList.contains(SHOW_CLASS))
Expand Down Expand Up @@ -295,6 +294,55 @@ describe('SlideScroll', () => {
});
});

it('should update to the right slide on scroll', () => {
return getAmpSlideScroll().then(obj => {
const ampSlideScroll = obj.ampSlideScroll;
const impl = ampSlideScroll.implementation_;
const showSlideSpy = sandbox.spy(impl, 'showSlide_');

impl.vsync_ = {
mutatePromise: cb => {
cb();
return {
then: cb2 => {
cb2();
},
};
},
mutate: cb => {
cb();
},
};

impl.slideWidth_ = 400;

// Move to slide 1 (from slide 0).
impl.showSlide_(1);
expect(showSlideSpy).to.be.calledWith(1);
expect(impl.snappingInProgress_).to.be.false;

//Move to slide 0 - via scrolling back.
impl.updateOnScroll_(1);
expect(showSlideSpy).to.be.calledWith(0);
expect(impl.slideIndex_).to.equal(0);

// Try scrolling Fwd and move to slide 1.
impl.updateOnScroll_(401);
expect(showSlideSpy).to.be.calledWith(1);
expect(impl.slideIndex_).to.equal(1);


impl.updateOnScroll_(700);
expect(showSlideSpy).to.be.calledWith(2);
expect(impl.slideIndex_).to.equal(2);

impl.showSlide_(4);
impl.updateOnScroll_(700);
expect(showSlideSpy).to.be.calledWith(4);
expect(impl.slideIndex_).to.equal(4);
});
});

describe('Looping', () => {
beforeEach(() => {
toggleExperiment(window, 'amp-slidescroll', true);
Expand Down Expand Up @@ -522,5 +570,48 @@ describe('SlideScroll', () => {
});
});

it('should update to the right slide on scroll', () => {
return getAmpSlideScroll(true).then(obj => {
const ampSlideScroll = obj.ampSlideScroll;
const impl = ampSlideScroll.implementation_;
const showSlideSpy = sandbox.spy(impl, 'showSlide_');

impl.vsync_ = {
mutate: cb => {
cb();
},
};

impl.slideWidth_ = 400;

// Move to slide 1 (from slide 0).
impl.showSlide_(1);
expect(showSlideSpy).to.be.calledWith(1);
expect(impl.snappingInProgress_).to.be.false;

//Move to slide 0 - via scrolling back.
impl.updateOnScroll_(1);
expect(showSlideSpy).to.be.calledWith(0);
expect(impl.slideIndex_).to.equal(0);

// Try scrolling Fwd and move a little fwd to stay in the same slide.
impl.updateOnScroll_(401);
expect(showSlideSpy).to.be.calledWith(0);
expect(impl.slideIndex_).to.equal(0);

impl.updateOnScroll_(700);
expect(showSlideSpy).to.be.calledWith(1);
expect(impl.slideIndex_).to.equal(1);

impl.showSlide_(4);
impl.updateOnScroll_(700);
expect(showSlideSpy).to.be.calledWith(0);
expect(impl.slideIndex_).to.equal(0);

impl.updateOnScroll_(1);
expect(showSlideSpy).to.be.calledWith(4);
expect(impl.slideIndex_).to.equal(4);
});
});
});
});
13 changes: 12 additions & 1 deletion test/manual/amp-slidescroll.amp.html
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,18 @@
</head>
<body>
<h1>amp #0</h1>
<amp-carousel id="carousel-3" width=400 height=300 type=slides controls loop>
<h1>No Loops for you</h1>
<amp-carousel id="carousel-1" width=400 height=300 type=slides controls>
<amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w300-h200-no" layout=fill></amp-img>
<amp-img src="https://lh3.googleusercontent.com/pSECrJ82R7-AqeBCOEPGPM9iG9OEIQ_QXcbubWIOdkY=w300-h200-no" width=100 height=70></amp-img>
<amp-img src="https://lh3.googleusercontent.com/Z4gtm5Bkxyv21Z2PtbTf95Clb9AE4VTR6olbBKYrenM=w300-h300-no" width=400 height=300></amp-img>
<amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w300-h200-no" layout=fill></amp-img>
<amp-img src="https://lh3.googleusercontent.com/pSECrJ82R7-AqeBCOEPGPM9iG9OEIQ_QXcbubWIOdkY=w300-h200-no" width=400 height=300></amp-img>
<amp-img src="https://lh3.googleusercontent.com/Z4gtm5Bkxyv21Z2PtbTf95Clb9AE4VTR6olbBKYrenM=w300-h300-no" width=400 height=300></amp-img>
</amp-carousel>

<h1>Loopy!</h1>
<amp-carousel id="carousel-2" width=400 height=300 type=slides controls loop>
<amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w300-h200-no" layout=fill></amp-img>
<amp-img src="https://lh3.googleusercontent.com/pSECrJ82R7-AqeBCOEPGPM9iG9OEIQ_QXcbubWIOdkY=w300-h200-no" width=100 height=70></amp-img>
<amp-img src="https://lh3.googleusercontent.com/Z4gtm5Bkxyv21Z2PtbTf95Clb9AE4VTR6olbBKYrenM=w300-h300-no" width=400 height=300></amp-img>
Expand Down