feat(carousel): improve Auto Scroll UX and enforce compatible transition settings#165
Open
milindmore22 wants to merge 2 commits into
Open
feat(carousel): improve Auto Scroll UX and enforce compatible transition settings#165milindmore22 wants to merge 2 commits into
milindmore22 wants to merge 2 commits into
Conversation
…prevent conflicts
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because no GitHub Actions runner was available. Make sure your repository has a runner available to run Copilot's review, or add a copilot-setup-steps.yml file specifying one with the runs-on attribute. See the docs for more details.
This PR prevents Carousel Auto Scroll from being enabled/initialized when the transition is set to fade, avoiding plugin conflicts during fade transitions.
Changes:
- Update editor logic to hide Auto Scroll controls for
fadeand forcibly disableautoScrollwhen switching tofade. - Update frontend/view initialization to skip the Auto Scroll plugin when
transition === 'fade', and adjust saved output accordingly. - Add/adjust unit tests and introduce a Jest mock for
embla-carousel-auto-scroll.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/js/setup.ts |
Adds a Jest mock for embla-carousel-auto-scroll to support new view tests. |
src/blocks/carousel/view.ts |
Prevents Auto Scroll plugin initialization when transition is fade. |
src/blocks/carousel/save.tsx |
Stops emitting Auto Scroll settings when transition is fade. |
src/blocks/carousel/edit.tsx |
Hides Auto Scroll panel for fade and disables autoScroll when switching to fade. |
src/blocks/carousel/__tests__/view.test.ts |
Adds a test asserting Auto Scroll isn’t included for fade transitions. |
src/blocks/carousel/__tests__/edit.test.tsx |
Updates editor tests to reflect UI/attribute changes around Auto Scroll and fade. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+67
to
+78
| jest.mock( 'embla-carousel-auto-scroll', () => ( { | ||
| __esModule: true, | ||
| default: jest.fn( () => ( { | ||
| name: 'autoScroll', | ||
| options: {}, | ||
| init: jest.fn(), | ||
| destroy: jest.fn(), | ||
| play: jest.fn(), | ||
| stop: jest.fn(), | ||
| isPlaying: jest.fn( () => false ), | ||
| } ) ), | ||
| } ) ); |
Comment on lines
+957
to
+962
| const lastCall = ( EmblaCarousel as unknown as jest.Mock ).mock.calls.at( -1 ); | ||
| expect( lastCall?.[ 2 ] ).toContainEqual( ( Fade as unknown as jest.Mock ).mock.results.at( -1 )?.value ); | ||
| const autoScrollMockResult = ( AutoScroll as unknown as jest.Mock ).mock.results.at( -1 )?.value; | ||
| if ( autoScrollMockResult ) { | ||
| expect( lastCall?.[ 2 ] ).not.toContainEqual( autoScrollMockResult ); | ||
| } |
Collaborator
|
@milindmore22 Let's flip this. Don't hide anything, and don't reset either attribute. Transition fade + auto-scroll on: keep initializing the auto-scroll plugin. That's what |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (1)
src/blocks/carousel/edit.tsx:1
- The PR description says Auto Scroll should be unavailable/disabled when
transition === 'fade'(and autoScroll reset tofalsewhen fade is chosen). The current implementation instead disables the Transition control whenever Auto Scroll is enabled, and it does not resetautoScrollwhen the user selectsfade. This is a behavior mismatch: either (a) keep Transition selectable and disable/hide Auto Scroll whentransition === 'fade'(resetting the attribute), or (b) update the PR description and adjust UX strings accordingly. Also, the help text 'Auto Scroll does not support transitions.' is misleading sinceslideis still valid—make it explicitly about thefadetransition.
import { __ } from '@wordpress/i18n';
Comment on lines
13
to
17
| emblaApi: contextApi, | ||
| canScrollPrev, | ||
| canScrollNext, | ||
| carouselOptions, | ||
| } = useContext( EditorCarouselContext ); |
| } } | ||
| type="button" | ||
| disabled={ ! canScrollPrev } | ||
| disabled={ ! carouselOptions?.autoScroll && ! canScrollPrev } |
| } } | ||
| type="button" | ||
| disabled={ ! canScrollNext } | ||
| disabled={ ! carouselOptions?.autoScroll && ! canScrollNext } |
| get canScrollPrev() { | ||
| const context = getContext<CarouselContext>(); | ||
| return context.canScrollPrev; | ||
| return context.autoScroll ? true : context.canScrollPrev; |
| get canScrollNext() { | ||
| const context = getContext<CarouselContext>(); | ||
| return context.canScrollNext; | ||
| return context.autoScroll ? true : context.canScrollNext; |
Comment on lines
322
to
+325
| <SelectControl | ||
| label={ __( 'Transition', 'rt-carousel' ) } | ||
| value={ transition } | ||
| disabled={ autoScroll } |
Comment on lines
330
to
+340
| onChange={ ( value ) => | ||
| setAttributes( { transition: value as CarouselAttributes[ 'transition' ] } ) | ||
| } | ||
| help={ __( | ||
| 'Choose how slides transition: sliding horizontally or cross-fading.', | ||
| 'rt-carousel', | ||
| ) } | ||
| help={ | ||
| autoScroll | ||
| ? __( 'Auto Scroll does not support transitions.', 'rt-carousel' ) | ||
| : __( | ||
| 'Choose how slides transition: sliding horizontally or cross-fading.', | ||
| 'rt-carousel', | ||
| ) | ||
| } |
| @@ -503,11 +508,17 @@ export default function Edit( { | |||
| <ToggleControl | |||
Comment on lines
+964
to
+975
| it( 'includes the AutoScroll plugin when transition is fade', () => { | ||
| const mockContext = createMockContext( { | ||
| transition: 'fade', | ||
| autoScroll: { | ||
| speed: 2, | ||
| direction: 'forward', | ||
| startDelay: 1000, | ||
| stopOnInteraction: true, | ||
| stopOnMouseEnter: false, | ||
| stopOnFocusIn: true, | ||
| }, | ||
| } ); |
Comment on lines
+1006
to
+1008
| const autoScrollMockResult = ( AutoScroll as unknown as jest.Mock ).mock.results.at( -1 )?.value; | ||
| expect( lastCall?.[ 2 ] ).toContainEqual( autoScrollMockResult ); | ||
| } finally { |
milindmore22
changed the title
Comment on lines
13
to
17
| emblaApi: contextApi, | ||
| canScrollPrev, | ||
| canScrollNext, | ||
| carouselOptions, | ||
| } = useContext( EditorCarouselContext ); |
| } } | ||
| type="button" | ||
| disabled={ ! canScrollPrev } | ||
| disabled={ ! carouselOptions?.autoScroll && ! canScrollPrev } |
| } } | ||
| type="button" | ||
| disabled={ ! canScrollNext } | ||
| disabled={ ! carouselOptions?.autoScroll && ! canScrollNext } |
Comment on lines
322
to
+325
| <SelectControl | ||
| label={ __( 'Transition', 'rt-carousel' ) } | ||
| value={ transition } | ||
| disabled={ autoScroll } |
Comment on lines
+333
to
+340
| help={ | ||
| autoScroll | ||
| ? __( 'Auto Scroll does not support transitions.', 'rt-carousel' ) | ||
| : __( | ||
| 'Choose how slides transition: sliding horizontally or cross-fading.', | ||
| 'rt-carousel', | ||
| ) | ||
| } |
Comment on lines
508
to
+521
| <ToggleControl | ||
| label={ __( 'Enable Auto Scroll', 'rt-carousel' ) } | ||
| checked={ autoScroll } | ||
| onChange={ ( value ) => setAttributes( { | ||
| autoScroll: value, | ||
| autoplay: value ? false : autoplay, | ||
| loop: ( value && autoScrollDirection === 'backward' ) ? true : loop, | ||
| } ) } | ||
| onChange={ ( value ) => { | ||
| const nextAttributes: Partial< CarouselAttributes > = { | ||
| autoScroll: value, | ||
| autoplay: value ? false : autoplay, | ||
| loop: ( value && autoScrollDirection === 'backward' ) ? true : loop, | ||
| }; | ||
| if ( value ) { | ||
| nextAttributes.transition = 'slide'; | ||
| } | ||
| setAttributes( nextAttributes ); | ||
| } } |
Comment on lines
+964
to
+975
| it( 'includes the AutoScroll plugin when transition is fade', () => { | ||
| const mockContext = createMockContext( { | ||
| transition: 'fade', | ||
| autoScroll: { | ||
| speed: 2, | ||
| direction: 'forward', | ||
| startDelay: 1000, | ||
| stopOnInteraction: true, | ||
| stopOnMouseEnter: false, | ||
| stopOnFocusIn: true, | ||
| }, | ||
| } ); |
Comment on lines
+1005
to
+1007
| expect( lastCall?.[ 2 ] ).toContainEqual( ( Fade as unknown as jest.Mock ).mock.results.at( -1 )?.value ); | ||
| const autoScrollMockResult = ( AutoScroll as unknown as jest.Mock ).mock.results.at( -1 )?.value; | ||
| expect( lastCall?.[ 2 ] ).toContainEqual( autoScrollMockResult ); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This pull request enhances the Carousel block by improving support for the Auto Scroll feature and ensuring a better user experience when it is enabled. The most important changes include UI updates to disable incompatible options when Auto Scroll is active, logic changes to enforce compatible settings, and expanded test coverage to ensure correct behavior.
UI/UX improvements for Auto Scroll:
Transitiondropdown in the carousel editor is now disabled when Auto Scroll is enabled, and a contextual help message is shown to inform users that transitions are not supported with Auto Scroll.'slide'to prevent incompatible settings.Logic updates:
canScrollPrevandcanScrollNextnow always returntruewhen Auto Scroll is enabled, ensuring consistent navigation behavior.Test coverage and mocks:
'slide'when enabling Auto Scroll, and that navigation and plugin logic behave correctly with Auto Scroll active. [1] [2] [3] [4] [5] [6]embla-carousel-auto-scrollplugin to support new and existing tests.embla-carousel-auto-scrollimport to the view tests to support plugin usage.Type of change
Related issue(s)
Closes #162
What changed
Breaking changes
Does this introduce a breaking change? If yes, describe the impact and migration path below.
Testing
Describe how this was tested.
Test details:
Screenshots / recordings
If applicable, add screenshots or short recordings.
Checklist