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

add back loop feature. #555

Merged
merged 4 commits into from
Oct 13, 2015
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
add back loop feature.
  • Loading branch information
erwinmombay committed Oct 13, 2015
commit 53d49a120f6c71b0defc0aa8cc7609b78169a46c
2 changes: 1 addition & 1 deletion extensions/amp-carousel/0.1/base-carousel.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ export class BaseCarousel extends AMP.BaseElement {
}

/**
* Calls `goCallback` and `setControlState` for transition behavior.
* Calls `goCallback` and `setControlsState` for transition behavior.
* @param {number} dir -1 or 1
* @param {boolean} animate
*/
Expand Down
9 changes: 9 additions & 0 deletions extensions/amp-carousel/0.1/slides.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ export class AmpSlides extends BaseCarousel {

/** @override */
buildCarousel() {
/** @private @const {boolean} */
this.isLooping_ = this.element.hasAttribute('loop');

/** @private {!Array<!Element>} */
this.slides_ = this.getRealChildren();
this.slides_.forEach((slide, i) => {
Expand Down Expand Up @@ -300,11 +303,17 @@ export class AmpSlides extends BaseCarousel {

/** @override */
hasPrev() {
if (this.isLooping_) {
return true;
}
return this.currentIndex_ != 0;
}

/** @override */
hasNext() {
if (this.isLooping_) {
return true;
}
return this.currentIndex_ != this.slides_.length - 1;
}
}
4 changes: 4 additions & 0 deletions extensions/amp-carousel/amp-carousel.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ unless only a single child is present.
Be aware that `type=carousel` does not currently support `layout=responsive`.
- `slides` - Shows a single slide at a time.

**loop**

If present, the user may advance past the final item,
which is followed by the first item again in the carousel.

#### Styling
- You may use the `amp-carousel` element selector to style it freely.
Expand Down
7 changes: 7 additions & 0 deletions test/fixtures/carousels.html
Original file line number Diff line number Diff line change
Expand Up @@ -55,5 +55,12 @@ <h1>AMP #0</h1>
<amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w400-h300-no" width=400 height=300></amp-img>
<amp-img src="https://lh3.googleusercontent.com/Z4gtm5Bkxyv21Z2PtbTf95Clb9AE4VTR6olbBKYrenM=w400-h300-no" width=400 height=300></amp-img>
</amp-carousel>

<p>Carousel type=slides w/ explicit controls and w/ looping</p>
<amp-carousel width=400 height=300 type=slides controls id="slides-4" loop>
<amp-img src="https://lh3.googleusercontent.com/pSECrJ82R7-AqeBCOEPGPM9iG9OEIQ_QXcbubWIOdkY=w400-h300-no" layout=fill></amp-img>
<amp-img src="https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-Rf78IofLb9QjS5_0mqsY1zEFc=w400-h300-no" width=400 height=300></amp-img>
<amp-img src="https://lh3.googleusercontent.com/Z4gtm5Bkxyv21Z2PtbTf95Clb9AE4VTR6olbBKYrenM=w400-h300-no" width=400 height=300></amp-img>
</amp-carousel>
</body>
</html>
30 changes: 29 additions & 1 deletion test/integration/test-amp-carousel.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import {createFixtureIframe, expectBodyToBecomeVisible} from
'../../testing/iframe.js';

describe.skip('amp-carousel', () => {
describe('amp-carousel', () => {

let fixture;
beforeEach(() => {
Expand Down Expand Up @@ -133,6 +133,34 @@ describe.skip('amp-carousel', () => {
});
});

it('(type=slides loop) should always have a prev and next button be ' +
'able to get past the first and last item', () => {
return fixture.awaitEvent('amp:load:start', 7).then(() => {
let amp = fixture.doc.querySelector('#slides-4');
expect(fixture.doc.body).to.have.class('amp-mode-mouse');
let prevBtn = amp.querySelector('.amp-carousel-button-prev');
let nextBtn = amp.querySelector('.amp-carousel-button-next');
expect(prevBtn).to.not.have.class('-amp-disabled');
expect(nextBtn).to.not.have.class('-amp-disabled');
nextBtn.click();
expect(prevBtn).to.not.have.class('-amp-disabled');
expect(nextBtn).to.not.have.class('-amp-disabled');
nextBtn.click();
expect(prevBtn).to.not.have.class('-amp-disabled');
expect(nextBtn).to.not.have.class('-amp-disabled');
nextBtn.click();
expect(prevBtn).to.not.have.class('-amp-disabled');
expect(nextBtn).to.not.have.class('-amp-disabled');
// TODO(erwinm): figure out why do we need 2 extra clicks here?
nextBtn.click();
expect(prevBtn).to.not.have.class('-amp-disabled');
expect(nextBtn).to.not.have.class('-amp-disabled');
nextBtn.click();
expect(prevBtn).to.not.have.class('-amp-disabled');
expect(nextBtn).to.not.have.class('-amp-disabled');
});
});

it('should not have any buttons visible when theres only a single ' +
'item', () => {
fixture.doc.body.classList.add('amp-mode-mouse');
Expand Down