Conversation
This would support issue rtCamp#88 - Add support for progress bar.
There was a problem hiding this comment.
Pull request overview
Adds a new “Carousel Progress” inner block to Carousel Kit and wires it to Embla’s scroll progress so the editor and frontend can render a progress bar similar to carousel dots.
Changes:
- Introduces a new
carousel-kit/carousel-progressblock (block.json, edit/save, styles). - Tracks
scrollProgressfrom Embla in both the frontend interactivity store and the editor context. - Registers the new block server-side for discovery/loading.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/blocks/carousel/view.ts |
Adds a progress-bar style callback and updates scrollProgress from Embla events. |
src/blocks/carousel/types.ts |
Extends carousel types to include progress block attributes and scrollProgress in CarouselContext. |
src/blocks/carousel/progress/style.scss |
Adds frontend styling for the progress bar. |
src/blocks/carousel/progress/save.tsx |
Adds frontend markup wired to interactivity callbacks. |
src/blocks/carousel/progress/edit.tsx |
Adds editor rendering using EditorCarouselContext. |
src/blocks/carousel/progress/index.ts |
Registers the new progress block. |
src/blocks/carousel/progress/block.json |
Declares the new progress block metadata. |
src/blocks/carousel/editor-context.ts |
Adds scrollProgress to the editor carousel context. |
src/blocks/carousel/edit.tsx |
Subscribes to Embla events to keep editor scrollProgress updated. |
inc/Plugin.php |
Registers the new carousel/progress block. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| scrollSnaps: { index: number }[]; | ||
| canScrollPrev: boolean; | ||
| canScrollNext: boolean; | ||
| scrollProgress: number; |
There was a problem hiding this comment.
CarouselContext now requires scrollProgress, but the initial CarouselContext object created in src/blocks/carousel/save.tsx (and several type tests) does not provide this field. This will break TypeScript compilation; either initialize scrollProgress in the saved context (e.g., 0) everywhere a CarouselContext literal is created, or make scrollProgress optional in the type and handle the default in view callbacks.
| scrollProgress: number; | |
| scrollProgress?: number; |
| const { scrollProgress, slideCount } = useContext( EditorCarouselContext ) as { scrollProgress?: number, slideCount?: number }; | ||
|
|
||
| // Hide if only one slide | ||
| if (slideCount === 1) return null; | ||
|
|
There was a problem hiding this comment.
slideCount is being read from EditorCarouselContext via an as { scrollProgress?: number, slideCount?: number } cast, but EditorCarouselContextType does not define/provide slideCount. As a result slideCount will always be undefined and the if (slideCount === 1) early return will never run. Compute slideCount via useSelect (similar to viewport/edit.tsx) or extend the editor context/provider to supply it.
| const { scrollProgress, slideCount } = useContext( EditorCarouselContext ) as { scrollProgress?: number, slideCount?: number }; | |
| // Hide if only one slide | |
| if (slideCount === 1) return null; | |
| const { scrollProgress } = useContext( EditorCarouselContext ) as { scrollProgress?: number }; |
| aria-valuemax={100} | ||
| data-wp-bind--style="callbacks.getProgressBarStyle" |
There was a problem hiding this comment.
For a determinate role="progressbar", ARIA requires the current value to be exposed (e.g. aria-valuenow). Right now the frontend markup only updates width via data-wp-bind--style, so assistive tech won’t get the progress value. Bind aria-valuenow (and optionally aria-valuetext) to a callback derived from scrollProgress so it stays in sync with the visual bar.
| aria-valuemax={100} | |
| data-wp-bind--style="callbacks.getProgressBarStyle" | |
| aria-valuemax={100} | |
| aria-valuenow={0} | |
| data-wp-bind--style="callbacks.getProgressBarStyle" | |
| data-wp-bind--aria-valuenow="callbacks.getProgressValue" |
| embla.on( 'scroll', () => { | ||
| context.scrollProgress = embla.scrollProgress(); | ||
| } ); |
There was a problem hiding this comment.
The Embla scroll event can fire very frequently; directly writing to the interactivity context on every event may cause excessive DOM/style updates. Consider throttling updates (e.g., via requestAnimationFrame) and/or only updating when the computed progress value actually changes beyond a small epsilon.
| getProgressBarStyle: () => { | ||
| const { scrollProgress } = getContext<CarouselContext>(); | ||
| return { | ||
| width: `${ ( scrollProgress || 0 ) * 100 }%`, | ||
| }; | ||
| }, |
There was a problem hiding this comment.
New callbacks.getProgressBarStyle is introduced but the existing src/blocks/carousel/__tests__/view.test.ts suite doesn’t exercise this callback or validate that scrollProgress is updated from Embla. Adding/adjusting tests for the new callback and state updates would help prevent regressions.
This would support issue #88 - Add support for progress bar.