From 7e38e93ab2e5099b815555c93f0daeae2d11fcb2 Mon Sep 17 00:00:00 2001 From: Alessandro Date: Fri, 9 Feb 2024 15:28:45 +0100 Subject: [PATCH] fix(carousel): remove check for scrolling (#1862) --- src/components/carousel/carousel.component.ts | 17 ++--- src/components/carousel/carousel.test.ts | 67 +++++++++++++------ 2 files changed, 51 insertions(+), 33 deletions(-) diff --git a/src/components/carousel/carousel.component.ts b/src/components/carousel/carousel.component.ts index 5e186fc9..34d097ea 100644 --- a/src/components/carousel/carousel.component.ts +++ b/src/components/carousel/carousel.component.ts @@ -476,7 +476,7 @@ export default class SlCarousel extends ShoelaceElement { this.scrollToSlide(nextSlide, prefersReducedMotion() ? 'auto' : behavior); } - private async scrollToSlide(slide: HTMLElement, behavior: ScrollBehavior = 'smooth') { + private scrollToSlide(slide: HTMLElement, behavior: ScrollBehavior = 'smooth') { const scrollContainer = this.scrollContainer; const scrollContainerRect = scrollContainer.getBoundingClientRect(); const nextSlideRect = slide.getBoundingClientRect(); @@ -484,16 +484,11 @@ export default class SlCarousel extends ShoelaceElement { const nextLeft = nextSlideRect.left - scrollContainerRect.left; const nextTop = nextSlideRect.top - scrollContainerRect.top; - // If the slide is already in view, don't need to scroll - if (nextLeft !== scrollContainer.scrollLeft || nextTop !== scrollContainer.scrollTop) { - scrollContainer.scrollTo({ - left: nextLeft + scrollContainer.scrollLeft, - top: nextTop + scrollContainer.scrollTop, - behavior - }); - - await waitForEvent(scrollContainer, 'scrollend'); - } + scrollContainer.scrollTo({ + left: nextLeft + scrollContainer.scrollLeft, + top: nextTop + scrollContainer.scrollTop, + behavior + }); } render() { diff --git a/src/components/carousel/carousel.test.ts b/src/components/carousel/carousel.test.ts index 42d693a7..01cfc8d1 100644 --- a/src/components/carousel/carousel.test.ts +++ b/src/components/carousel/carousel.test.ts @@ -1,6 +1,6 @@ import '../../../dist/shoelace.js'; import { clickOnElement, dragElement, moveMouseOnElement } from '../../internal/test.js'; -import { expect, fixture, html, oneEvent } from '@open-wc/testing'; +import { expect, fixture, html, nextFrame, oneEvent } from '@open-wc/testing'; import { map } from 'lit/directives/map.js'; import { range } from 'lit/directives/range.js'; import { resetMouse } from '@web/test-runner-commands'; @@ -8,10 +8,16 @@ import sinon from 'sinon'; import type SlCarousel from './carousel.js'; describe('', () => { + const sandbox = sinon.createSandbox(); + afterEach(async () => { await resetMouse(); }); + afterEach(() => { + sandbox.restore(); + }); + it('should render a carousel with default configuration', async () => { // Arrange const el = await fixture(html` @@ -34,15 +40,11 @@ describe('', () => { let clock: sinon.SinonFakeTimers; beforeEach(() => { - clock = sinon.useFakeTimers({ + clock = sandbox.useFakeTimers({ now: new Date() }); }); - afterEach(() => { - clock.restore(); - }); - it('should scroll forwards every `autoplay-interval` milliseconds', async () => { // Arrange const el = await fixture(html` @@ -52,7 +54,7 @@ describe('', () => { Node 3 `); - sinon.stub(el, 'next'); + sandbox.stub(el, 'next'); await el.updateComplete; @@ -73,7 +75,7 @@ describe('', () => { Node 3 `); - sinon.stub(el, 'next'); + sandbox.stub(el, 'next'); await el.updateComplete; @@ -96,7 +98,7 @@ describe('', () => { Node 3 `); - sinon.stub(el, 'next'); + sandbox.stub(el, 'next'); await el.updateComplete; @@ -183,7 +185,7 @@ describe('', () => { Node 3 `); - sinon.stub(el, 'goToSlide'); + sandbox.stub(el, 'goToSlide'); await el.updateComplete; // Act @@ -473,7 +475,7 @@ describe('', () => { `); const nextButton: HTMLElement = el.shadowRoot!.querySelector('.carousel__navigation-button--next')!; - sinon.stub(el, 'next'); + sandbox.stub(el, 'next'); await el.updateComplete; @@ -496,7 +498,7 @@ describe('', () => { `); const nextButton: HTMLElement = el.shadowRoot!.querySelector('.carousel__navigation-button--next')!; - sinon.stub(el, 'next'); + sandbox.stub(el, 'next'); el.goToSlide(2, 'auto'); await oneEvent(el.scrollContainer, 'scrollend'); @@ -560,7 +562,7 @@ describe('', () => { await el.updateComplete; const previousButton: HTMLElement = el.shadowRoot!.querySelector('.carousel__navigation-button--previous')!; - sinon.stub(el, 'previous'); + sandbox.stub(el, 'previous'); await el.updateComplete; @@ -584,7 +586,7 @@ describe('', () => { `); const previousButton: HTMLElement = el.shadowRoot!.querySelector('.carousel__navigation-button--previous')!; - sinon.stub(el, 'previous'); + sandbox.stub(el, 'previous'); await el.updateComplete; // Act @@ -632,19 +634,27 @@ describe('', () => { it('should scroll the carousel to the next slide', async () => { // Arrange const el = await fixture(html` - + Node 1 Node 2 Node 3 `); - sinon.stub(el, 'goToSlide'); - await el.updateComplete; + sandbox.spy(el, 'goToSlide'); + const expectedCarouselItem: HTMLElement = el.querySelector('sl-carousel-item:nth-child(2)')!; // Act el.next(); + await oneEvent(el.scrollContainer, 'scrollend'); + await el.updateComplete; - expect(el.goToSlide).to.have.been.calledWith(2); + const containerRect = el.scrollContainer.getBoundingClientRect(); + const itemRect = expectedCarouselItem.getBoundingClientRect(); + + // Assert + expect(el.goToSlide).to.have.been.calledWith(1); + expect(itemRect.top).to.be.equal(containerRect.top); + expect(itemRect.left).to.be.equal(containerRect.left); }); }); @@ -652,19 +662,32 @@ describe('', () => { it('should scroll the carousel to the previous slide', async () => { // Arrange const el = await fixture(html` - + Node 1 Node 2 Node 3 `); - sinon.stub(el, 'goToSlide'); - await el.updateComplete; + const expectedCarouselItem: HTMLElement = el.querySelector('sl-carousel-item:nth-child(1)')!; + + el.goToSlide(1); + + await oneEvent(el.scrollContainer, 'scrollend'); + await nextFrame(); + + sandbox.spy(el, 'goToSlide'); // Act el.previous(); + await oneEvent(el.scrollContainer, 'scrollend'); - expect(el.goToSlide).to.have.been.calledWith(-2); + const containerRect = el.scrollContainer.getBoundingClientRect(); + const itemRect = expectedCarouselItem.getBoundingClientRect(); + + // Assert + expect(el.goToSlide).to.have.been.calledWith(0); + expect(itemRect.top).to.be.equal(containerRect.top); + expect(itemRect.left).to.be.equal(containerRect.left); }); });