feat: add arrow position options (below/overlay/outside) to Controls block - #187
feat: add arrow position options (below/overlay/outside) to Controls block#187sanketio wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a new Controls-block “Position” option to let carousel arrows render below (default), overlaying the slide edges, or outside in side gutters, while keeping the default output byte-identical for existing content.
Changes:
- Introduces a new
positionattribute forrt-carousel/carousel-controlsplus editor UI (SelectControl) to configure it. - Adds a shared
getPositionClassNamehelper used by botheditandsaveto avoid class drift. - Adds CSS for overlay/outside positioning (including
:has()-based gutters) and updates/extends unit tests.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/blocks/carousel/types.ts | Adds the position attribute typing for Controls. |
| src/blocks/carousel/controls/block.json | Declares the new position attribute with default below. |
| src/blocks/carousel/controls/get-position-class.ts | New helper to compute wrapper classes consistently. |
| src/blocks/carousel/controls/edit.tsx | Adds Inspector control + uses helper-driven class name. |
| src/blocks/carousel/controls/save.tsx | Uses helper-driven class name in saved markup. |
| src/blocks/carousel/controls/style.scss | Implements overlay/outside arrow positioning and gutters. |
| src/blocks/carousel/controls/tests/get-position-class.test.ts | Unit tests for the helper. |
| src/blocks/carousel/controls/tests/edit.test.tsx | Updates editor tests for new UI + class behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export type CarouselControlsPosition = 'below' | 'overlay' | 'outside'; | ||
| export type CarouselControlsAttributes = { position: CarouselControlsPosition }; |
There was a problem hiding this comment.
block.json declares position with default: "below", and WordPress applies attribute defaults at parse time — so save/edit always receive a defined position, including older blocks that serialized nothing. The required type matches that runtime contract. The helper accepting position? is deliberate defensiveness for standalone calls, not a mismatch. Leaving as-is.
| onChange={ ( value ) => | ||
| setAttributes( { | ||
| position: value as CarouselControlsPosition, | ||
| } ) | ||
| } |
There was a problem hiding this comment.
The SelectControl options are a fixed set (below/overlay/outside) and single-select onChange is typed (value: string) => void, so the only values reaching setAttributes are those three. The cast just narrows string to the union — the idiomatic WP pattern. A runtime fallback would not guard against anything the UI can produce, so leaving it.
| .rt-carousel:not([data-axis="y"]) .rt-carousel-controls.is-position-overlay, | ||
| .rt-carousel:not([data-axis="y"]) .rt-carousel-controls.is-position-outside { | ||
| position: absolute; | ||
| inset: 0; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: space-between; | ||
| pointer-events: none; | ||
| } |
There was a problem hiding this comment.
Good catch — Embla transforms the track, which creates a stacking context, so relying on paint order alone is fragile. Fixed in 608731e by adding z-index: var(--rt-carousel-control-z, 1) to the absolutely-positioned overlay/outside wrapper, so the arrows reliably sit above slide content (overridable via the variable, matching the block's other control tokens).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (6)
src/blocks/carousel/types.ts:37
- Making
positionrequired inCarouselControlsAttributesconflicts with the explicitly supported back-compat path (older blocks serialize noposition, andgetPositionClassName()acceptsposition?). Consider changing the type toposition?: CarouselControlsPosition(or a union that allowsundefined) so TS reflects the runtime/back-compat contract and callers don’t need to fabricate a default just to satisfy the type.
export type CarouselControlsPosition = 'below' | 'overlay' | 'outside';
export type CarouselControlsAttributes = { position: CarouselControlsPosition };
src/blocks/carousel/controls/edit.tsx:82
SelectControl’sonChangevalue can be broader than the literal union (e.g.,string, potentially empty/undefined depending on component behavior). Blind-casting (as CarouselControlsPosition) can let invalid values into attributes. Prefer a small validation/mapping step (e.g., accept only'below' | 'overlay' | 'outside', otherwise fall back to'below') so the attribute remains sound at runtime.
onChange={ ( value ) =>
setAttributes( {
position: value as CarouselControlsPosition,
} )
}
src/blocks/carousel/controls/style.scss:72
- For
outside, the gutter relies on:has(). In browsers without:has()support, the.rt-carousel-controls.is-position-outsidestill becomesposition: absolute; inset: 0;but the carousel won’t get padding, so the arrows can end up overlaying content (and at the very edge, since outside has no inset padding). Consider adding a non-:has()fallback foroutside(e.g., apply apadding-inlineinset on the controls itself foris-position-outside, and optionally gate the:has()gutter enhancement behind@supports selector(:has(*))).
.rt-carousel:not([data-axis="y"]) .rt-carousel-controls.is-position-overlay,
.rt-carousel:not([data-axis="y"]) .rt-carousel-controls.is-position-outside {
position: absolute;
inset: 0;
z-index: var(--rt-carousel-control-z, 1);
display: flex;
align-items: center;
justify-content: space-between;
pointer-events: none;
}
src/blocks/carousel/controls/style.scss:90
- For
outside, the gutter relies on:has(). In browsers without:has()support, the.rt-carousel-controls.is-position-outsidestill becomesposition: absolute; inset: 0;but the carousel won’t get padding, so the arrows can end up overlaying content (and at the very edge, since outside has no inset padding). Consider adding a non-:has()fallback foroutside(e.g., apply apadding-inlineinset on the controls itself foris-position-outside, and optionally gate the:has()gutter enhancement behind@supports selector(:has(*))).
.rt-carousel:not([data-axis="y"]):has(
.rt-carousel-controls.is-position-outside
) {
padding-inline: var(--rt-carousel-control-gutter, 3rem);
}
src/blocks/carousel/controls/tests/edit.test.tsx:8
- This test treats
SelectControlas a Jest mock and inspects.mock.calls, but this file doesn’t mock@wordpress/componentsin the shown changes. If the mock is provided only via global test setup, the test becomes order/config-dependent and harder to understand. Prefer mocking@wordpress/componentsin this test (exportingSelectControl: jest.fn()), or assert behavior via DOM queries/events instead of relying on implementation-level.mock.calls.
import { SelectControl } from '@wordpress/components';
src/blocks/carousel/controls/tests/edit.test.tsx:109
- This test treats
SelectControlas a Jest mock and inspects.mock.calls, but this file doesn’t mock@wordpress/componentsin the shown changes. If the mock is provided only via global test setup, the test becomes order/config-dependent and harder to understand. Prefer mocking@wordpress/componentsin this test (exportingSelectControl: jest.fn()), or assert behavior via DOM queries/events instead of relying on implementation-level.mock.calls.
const positionCall = (
SelectControl as unknown as jest.Mock
).mock.calls.find( ( call ) => call[ 0 ].label === 'Position' );
|
Thanks Copilot — addressing the two new points from the latest re-review (both surfaced as suppressed comments):
|
Summary
Adds a Position option to the Carousel Controls block so the prev/next arrows can sit Below the slides (current default, unchanged), Overlay on the slide edges, or Outside flanking the slides. This is the common "arrows left/right of / over the cards" slider layout requested in #143.
The setting lives on the Controls block as a new
positionattribute, exposed via aSelectControlin the block inspector. A shared pure helper derives the wrapper class soeditandsavecan't drift, and CSS positions the arrows against the ancestor.rt-carousel(alreadyposition: relative), using:has()for the Outside gutter — the same:has()pattern already used for tabs mode.Type of change
Related issue(s)
Closes #143
What changed
positionattribute (belowdefault /overlay/outside) onrt-carousel/carousel-controlswith a labelled PositionSelectControlin the inspector (plus a hint to place Controls as a direct child of the carousel for overlay/outside).getPositionClassNamehelper used by bothedit.tsxandsave.tsx→belowemits onlyrt-carousel-controls; overlay/outside addis-position-overlay/is-position-outside.controls/style.scss): overlay floats arrows over the slide edges (absolute, vertically centered); outside adds side gutters via:has()so arrows flank the slides. Horizontal-only (:not([data-axis="y"])); RTL handled by existing logical-property + icon-flip rules. Tunable via--rt-carousel-control-inset/--rt-carousel-control-gutter/--rt-carousel-control-gutter-mobile. Overlay/outside also setz-index: var(--rt-carousel-control-z, 1)so the arrows reliably stack above slide content (Embla transforms the track, creating a stacking context) — added per review feedback.Breaking changes
Default is
below, which is not serialized and emits byte-identical markup to the current output (class="rt-carousel-controls", no modifier). Existing saved Controls blocks re-parse as valid — no block-validation errors, no deprecation entry, no forced re-save. New CSS activates only on the opt-in classes, so sites that just update the plugin look pixel-for-pixel the same.Testing
Describe how this was tested.
:has()for the Outside gutter — already used by tabs mode; overlay does not use:has())Test details — run from the plugin root after checking out this branch:
Manual (in a WordPress editor): insert a Carousel → select the Controls block → set Position to Overlay then Outside, and verify on the front end:
Full step-by-step How-to-test lives on the issue: #143.
Note on overlay centering
Overlay/outside arrows center on the slide track (
.rt-carousel/ viewport). If slide media carries a trailing margin — e.g. WordPress core/theme's:where(figure){margin:0 0 1em}on image slides — the track is taller than the visible media, so the arrows read as ~8px low. This is theme/content styling (the author controls it), not the feature's CSS, which adds no vertical margin/padding. Slides that fill their height (Cover blocks, vertically-centered content) don't show it.Screenshots / recordings
Verified on a local site with three carousels (below / overlay / outside); the compiled overlay/outside rules are served and applied on the front end. (Local dev env — no public URL to link.)
Checklist