Add responsive carousel direction controls#24
Conversation
goldenapples
left a comment
There was a problem hiding this comment.
I like some of the added robustness, like adding default values for padding and heights.
I'm not opposed to defining the defaults in the config file, but I think it would be a bit more elegant if each of the place that's imported could just import from the defaults in block.json instead. There could well be a reason that doesn't make sense, but I'm just thinking.
If we could remove the breakpoints for auto-scroll, that would simplify this PR a good bit. Is it really necessary to specify different autoscroll behavior based on viewport?
| "tablet": true, | ||
| "mobile": true | ||
| } | ||
| }, |
There was a problem hiding this comment.
This attribute feels a little inconsistant, in that there's an "autoScroll" attribute defaulting to false, and then "autoScrollBreakpoints" which defaults to true at each breakpoint.
Maybe I'm just not thinking of the use cases where you would be setting autoscroll differently at different breakpoints. If this is a common use case, would it make sense to just let "autoScroll" take an object with three breakpoints (I know that is breaking backwards compatibility, but since this feature was just added in the last PR to this plugin it should be manageable to handle the update.)
There was a problem hiding this comment.
Agreed. I removed autoScrollBreakpoints and kept autoScroll as the single public setting.
The resize behavior still works. When Auto Scroll is enabled, Splide still gets breakpoint options that match the active direction. Vertical breakpoints avoid autoWidth. Horizontal breakpoints can still use it.
I also validated desktop to mobile resize behavior against the updated markup.
| desktop: 'ltr', | ||
| tablet: 'ltr', | ||
| mobile: 'ltr', | ||
| }; |
There was a problem hiding this comment.
It seems awkward to have to handle these default values separately from the block attribute defaults. Can we just import the defaults from block.json here?
There was a problem hiding this comment.
I looked at this and left the shared defaults in config.mjs for now.
This helper is used by the browser bundle and by Node tests. Direct JSON imports need import attributes in native ESM. That makes the test and tooling path more brittle.
I did remove the extra Auto Scroll breakpoint default.
| DEFAULT_DIRECTION, | ||
| DEFAULT_PADDING, | ||
| DEFAULT_RESPONSIVE_BOOLEAN, | ||
| DEFAULT_RESPONSIVE_LENGTH, |
There was a problem hiding this comment.
If there's a way to import these from block.json instead, that would seem better to me.
There was a problem hiding this comment.
Same reasoning here.
view.js imports the shared defaults from config.mjs. That keeps the frontend and tests on the same constants without adding direct JSON imports in this path.
I would be open to centralizing this later. I would rather avoid adding JSON import tooling risk in this PR.
|
Thanks. This pushed me to simplify the PR. Auto Scroll is back to one public setting. The PR still keeps responsive direction and the Splide breakpoint options needed for direction changes. I validated this against the updated carousel markup. Desktop initializes vertically and scrolls. After resizing to mobile, it switches to horizontal and keeps scrolling with visible slides. |
goldenapples
left a comment
There was a problem hiding this comment.
I think this all looks good! I'll merge and cut a new release.
Context
This adds responsive orientation support to the Carousel block so a carousel can use different directions and sizing rules at desktop, tablet, and mobile breakpoints.
Before this change, carousel direction and vertical sizing were effectively global settings. That made mixed-orientation layouts difficult to support cleanly: a vertical carousel needed a stable viewport height and slide height, while a horizontal breakpoint needed those vertical-only dimensions cleared again. It also left room for stale Splide state after resizing between directions, especially when Auto Scroll was enabled.
Keeping this behavior inside
hm-carousel-blockgives package consumers a reusable block capability instead of requiring custom JavaScript, wrapper blocks, or one-off theme fixes whenever a carousel needs to change orientation responsively.What Changed
directioncontrols for desktop, tablet, and mobile.heightandfixedHeightattributes so vertical carousels can define a stable viewport and slide height per breakpoint.autoWidthlimited to horizontal Auto Scroll carousels so vertical carousels do not inherit horizontal sizing assumptions.Review Notes
The main implementation path is:
src/blocks/carousel/edit.jsexposes the responsive controls in the editor.src/blocks/carousel/config.mjsconverts saved block attributes into Splide options and breakpoint options.src/blocks/carousel/view.jspasses parsed block settings into the config helper and handles Splide refresh behavior when responsive options change.tests/carousel-config.test.mjscovers the config behavior for direction, height inheritance/clearing, padding, thumbnail pagination, and Auto Scroll.The important behavior to review is that vertical-only settings stay scoped to vertical breakpoints, while horizontal breakpoints return to normal horizontal carousel sizing without carrying over stale height or transform state.
Release Notes
This PR changes the package API by adding responsive object shapes for
direction,height, andfixedHeight. Existing scalar settings continue to be normalized through the carousel config path.After this PR merges, publish or tag an updated package version through the normal release flow so projects using the package can depend on a released version rather than a temporary branch build.
Validation
Package checks run for this PR:
npm testnpx wp-scripts lint-js src/blocks/carousel/view.jsnpm run buildEarlier package checks also covered the broader carousel config changes:
npx wp-scripts lint-js src/blocks/carousel/config.mjs src/blocks/carousel/edit.js src/blocks/carousel/view.js tests/carousel-config.test.mjsKnown existing lint state:
npm run lint:cssreports existing stylelint issues in generated Splide CSS underbuild-vendor/and untouchedsrc/blocks/carousel/view.css.