[In progress] Add a Carousel block #49
Conversation
This also adds some very rough styles and a view script that doesn't do anything yet.
These may seem heavy-handed, but they're the lightest I was able to come up with while still overriding some competing core styles.
Add initial gallery carousel handling
nate-allen
left a comment
nate-allen
left a comment
There was a problem hiding this comment.
Just a couple minor things that need to be updated. Thank you
The "Left" position needs a bit more work. I don't want to get too hung up on that right now, though.
Add initial arrow button position handling
Adding a `ref` to the block itself interferes with its ability to be selected, so this shuffles things around a bit and adds an empty div for the sole purpose of holding the ref.
This applies `gap` only in cases where its needed so extra spacing isn't hanging out otherwise.
Carousel Block: Add support for arbitrary cards
It's a bummer to have to replicate all this, but it affords the ability to change things without invalidating saved blocks.
Carousel Block: Add PHP rendering
Without this, the `classes/` directory is not included with the build. See https://developer.wordpress.org/block-editor/reference-guides/packages/packages-scripts/#plugin-zip
Carousel: Prevent wp.org updates; package all files with `wp-scripts plugin-zip`
Carousel Block: Add initial drag/swipe handling
Carousel Block: improve title support for screen reader users
This is redundant, and can break certain embeds, etc.
Carousel Block: remove escaping from carousel content
Carousel Block: add an animation speed control
Carousel block: refine WC product collection block handling
WalkthroughAdds a new WordPress plugin "Carousel" under the carousel/ directory with build and source scaffolding. Introduces PHP bootstrap (plugin file and self-update class), package.json, .editorconfig, and .gitignore. Implements three blocks (wpcomsp/carousel, wpcomsp/carousel-nav, wpcomsp/carousel-pagination) including block.json, editor and save components, utilities, styles, variations, placeholder, and a comprehensive frontend view script implementing drag/keyboard navigation, multiple animation modes, pagination/navigation controls, and accessibility. Includes readme and build assets. Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
♻️ Duplicate comments (1)
carousel/carousel.php (1)
4-4: Address the previous feedback about updating the description.As noted in the previous review comments, the current description "Display a horizontal series of content" is quite generic. Consider making it more descriptive and specific to the carousel functionality.
- * Description: Display a horizontal series of content. + * Description: Create responsive image carousels, post sliders, and product galleries with navigation and pagination controls.
🧹 Nitpick comments (13)
carousel/package.json (1)
18-20: Consider more specific file inclusion pattern.The current pattern
"[^.]*"includes all non-hidden files, which might be overly broad for distribution.Consider a more specific pattern to exclude unnecessary files:
- "files": [ - "[^.]*" - ], + "files": [ + "build/**", + "src/**", + "*.php", + "*.json", + "readme.txt" + ],carousel/readme.txt (4)
33-39: Remove placeholder FAQ content before production release.The FAQ section contains generic placeholder questions and answers that should be replaced with actual, relevant content or removed entirely.
-= A question that someone might have = - -An answer to that question. - -= What about foo bar? = - -Answer to foo bar dilemma.
43-47: Update screenshot descriptions with actual content.The screenshot descriptions contain boilerplate text explaining how screenshots work rather than describing the actual carousel screenshots.
Replace the generic screenshot description with actual descriptions of your carousel screenshots, or remove the section if no screenshots are provided yet.
9-20: Polish the description for clarity & SEOThe list is clear, but one-sentence preamble repeats line 9. Consider merging and adding a closing sentence that highlights accessibility (drag/swipe, keyboard, ARIA).
22-30: Fix list numbering & minor wordingSequential numbering renders fine on WP.org but using distinct numbers avoids diff noise and improves readability.
-1. Upload the plugin files to the `/wp-content/plugins/carousel` directory, or install the plugin through the WordPress plugins screen directly. -1. Activate the plugin through the 'Plugins' screen in WordPress +1. Upload the plugin files to `/wp-content/plugins/carousel`, or install it via the Plugins screen. +2. Activate the plugin from the Plugins screen in WordPress.carousel/src/carousel/placeholder.js (1)
59-59: Consider more robust title handling.The string replacement
variation.title.replace( ' Carousel', '' )assumes all variation titles end with " Carousel". Consider a more robust approach that handles edge cases.-{ variation.title.replace( ' Carousel', '' ) } +{ variation.title.replace( /\s+Carousel$/i, '' ) || variation.title }This uses a regex to match " Carousel" at the end of the string (case-insensitive) and falls back to the original title if no match is found.
carousel/src/carousel-nav/style.css (1)
35-38: Consider consistency with pagination disabled state styling.The navigation uses
pointer-events: nonefor disabled buttons, while the pagination CSS usescursor: not-allowed. Consider standardizing the approach across both components.For consistency with the pagination component, consider:
&[aria-disabled="true"] { opacity: 0.5; - pointer-events: none; + cursor: not-allowed; }carousel/classes/class-wpcomsp-blocks-self-update.php (2)
54-56: Update the comment to match the actual URL.The comment mentions "opsoasis.mystagingwebsite.com" but the actual URL used is "opsoasis.wpspecialprojects.com".
-// Ask opsoasis.mystagingwebsite.com if there's an update. +// Ask opsoasis.wpspecialprojects.com if there's an update.
65-66: Update comment to match the actual URL.Similar to the previous comment, this one also references the wrong domain.
-// Bail if this plugin wasn't found on opsoasis.mystagingwebsite.com. +// Bail if this plugin wasn't found on opsoasis.wpspecialprojects.com.carousel/src/carousel-nav/common.js (1)
74-125: Consider more descriptive alt text for custom icons.The implementation is solid with good accessibility. However, when using custom icons, the alt text is generic. Consider allowing users to provide custom alt text for their uploaded icons to improve accessibility.
Would you like me to suggest an implementation that allows custom alt text for uploaded icons?
carousel/src/carousel/view.js (3)
23-25: Consider using data attributes for more robust element selection.The current selectors are complex and tightly coupled to specific block class names. This approach could be fragile if block implementations change.
Consider using data attributes for more maintainable selectors:
- track: carousel.querySelector( - '.wp-block-gallery, .wp-block-post-template, .wp-block-wpcomsp-carousel-track, .wc-block-product-template' - ), + track: carousel.querySelector('[data-carousel-track]'),And similarly for slides:
- carousel.querySelectorAll( - '.wp-block-gallery > .wp-block-image, .wp-block-post-template > .wp-block-post, .wp-block-wpcomsp-carousel-track > *, .wc-block-product-template .wc-block-product' - ) + carousel.querySelectorAll('[data-carousel-slide]')This would require adding the corresponding data attributes in the block's save function.
Also applies to: 39-41
94-94: Consider expanding the focusable elements selector.The current selector might miss some interactive elements like
[contenteditable],[tabindex]:not([tabindex="-1"]), or custom elements with role="button".- const focusable = 'a, button, input, select, textarea'; + const focusable = 'a, button, input, select, textarea, [contenteditable], [tabindex]:not([tabindex="-1"]), [role="button"]';
593-599: Complete the infinite carousel setup implementation.The function contains only a placeholder comment and an empty conditional block.
Would you like me to help implement the infinite carousel setup logic or create an issue to track this TODO?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
carousel/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (31)
carousel/.editorconfig(1 hunks)carousel/.gitignore(1 hunks)carousel/carousel.php(1 hunks)carousel/classes/class-wpcomsp-blocks-self-update.php(1 hunks)carousel/package.json(1 hunks)carousel/readme.txt(1 hunks)carousel/src/carousel-nav/block.json(1 hunks)carousel/src/carousel-nav/common.js(1 hunks)carousel/src/carousel-nav/edit.js(1 hunks)carousel/src/carousel-nav/editor.css(1 hunks)carousel/src/carousel-nav/index.js(1 hunks)carousel/src/carousel-nav/save.js(1 hunks)carousel/src/carousel-nav/style.css(1 hunks)carousel/src/carousel-pagination/block.json(1 hunks)carousel/src/carousel-pagination/common.js(1 hunks)carousel/src/carousel-pagination/edit.js(1 hunks)carousel/src/carousel-pagination/editor.css(1 hunks)carousel/src/carousel-pagination/index.js(1 hunks)carousel/src/carousel-pagination/save.js(1 hunks)carousel/src/carousel-pagination/style.css(1 hunks)carousel/src/carousel/block.json(1 hunks)carousel/src/carousel/common.js(1 hunks)carousel/src/carousel/edit.js(1 hunks)carousel/src/carousel/editor.css(1 hunks)carousel/src/carousel/index.js(1 hunks)carousel/src/carousel/placeholder.js(1 hunks)carousel/src/carousel/save.js(1 hunks)carousel/src/carousel/style.css(1 hunks)carousel/src/carousel/variations.js(1 hunks)carousel/src/carousel/view.css(1 hunks)carousel/src/carousel/view.js(1 hunks)
🧬 Code Graph Analysis (3)
carousel/carousel.php (1)
carousel/classes/class-wpcomsp-blocks-self-update.php (3)
WPCOMSP_Blocks_Self_Update(13-80)get_instance(22-28)hooks(33-35)
carousel/src/carousel-nav/edit.js (3)
carousel/src/carousel-pagination/edit.js (5)
Edit(17-87)props(18-18)attributes(19-19)buttonColors(20-20)innerBlocksProps(24-31)carousel/src/carousel-nav/common.js (5)
attributes(16-16)attributes(60-60)buttonColors(17-17)getHTMLAttributes(15-50)navigationButtons(59-126)carousel/src/carousel-nav/save.js (2)
attributes(8-8)innerBlocksProps(10-15)
carousel/src/carousel/variations.js (1)
carousel/src/carousel/placeholder.js (1)
variations(20-23)
🪛 Biome (1.9.4)
carousel/src/carousel/edit.js
[error] 113-113: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 114-114: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 116-116: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 117-117: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 120-135: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 138-151: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 153-157: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🧰 Additional context used
🧬 Code Graph Analysis (3)
carousel/carousel.php (1)
carousel/classes/class-wpcomsp-blocks-self-update.php (3)
WPCOMSP_Blocks_Self_Update(13-80)get_instance(22-28)hooks(33-35)
carousel/src/carousel-nav/edit.js (3)
carousel/src/carousel-pagination/edit.js (5)
Edit(17-87)props(18-18)attributes(19-19)buttonColors(20-20)innerBlocksProps(24-31)carousel/src/carousel-nav/common.js (5)
attributes(16-16)attributes(60-60)buttonColors(17-17)getHTMLAttributes(15-50)navigationButtons(59-126)carousel/src/carousel-nav/save.js (2)
attributes(8-8)innerBlocksProps(10-15)
carousel/src/carousel/variations.js (1)
carousel/src/carousel/placeholder.js (1)
variations(20-23)
🪛 Biome (1.9.4)
carousel/src/carousel/edit.js
[error] 113-113: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 114-114: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 116-116: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 117-117: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 120-135: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 138-151: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 153-157: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (44)
carousel/.gitignore (1)
1-31: LGTM! Comprehensive .gitignore for WordPress block development.The .gitignore file covers all the essential patterns for a WordPress block plugin project, including build artifacts, dependencies, cache files, and environment variables. The patterns are well-organized and follow standard conventions.
carousel/src/carousel-nav/editor.css (1)
1-3: Good editor UX approach.Disabling pointer events on navigation buttons in the editor is the right approach - it prevents users from trying to interact with non-functional navigation during block editing while maintaining visual consistency.
carousel/src/carousel-pagination/editor.css (1)
1-3: Consistent editor behavior with navigation.Good consistency with the navigation block's editor approach - pagination buttons are appropriately non-interactive in the editor environment.
carousel/src/carousel-pagination/index.js (1)
1-13: Standard WordPress block registration implementation.The block registration follows WordPress best practices perfectly - clean imports, proper use of metadata from block.json, and standard edit/save component structure.
carousel/.editorconfig (1)
1-18: Well-configured editor settings aligned with WordPress standards.The .editorconfig properly implements WordPress Coding Standards with appropriate settings for charset, line endings, indentation, and file-specific overrides for YAML. The configuration will ensure consistent formatting across different editors and IDEs.
carousel/src/carousel-nav/index.js (1)
1-15: LGTM! Clean WordPress block registration.The block registration follows WordPress best practices with proper imports, metadata usage, and component structure.
carousel/src/carousel-pagination/save.js (1)
7-18: Excellent WordPress save component implementation.The save function follows WordPress block development best practices with proper hooks usage, safe property access, and clean separation of concerns.
carousel/src/carousel-nav/save.js (1)
7-20: Consistent and well-structured save component.The implementation mirrors the pagination save component with appropriate differences for navigation functionality. Good use of WordPress hooks and consistent patterns.
carousel/src/carousel/index.js (1)
1-15: Clean main carousel block registration.The block registration follows WordPress standards with proper component imports. The inclusion of variations is good for providing preset carousel configurations.
carousel/src/carousel/save.js (1)
1-13: LGTM! Well-structured WordPress block save component.The implementation correctly follows WordPress block development patterns:
- Proper use of
useInnerBlocksProps.save()anduseBlockProps.save()- Clean destructuring of children and props
- Appropriate use of the
getHTMLAttributesutility for generating block attributescarousel/readme.txt (2)
1-30: Well-structured plugin documentation.The readme follows WordPress plugin directory standards with clear description of carousel functionality and proper metadata.
49-53: Changelog uses correct format – good startVersion entry is concise and follows WP.org expectations.
carousel/src/carousel-pagination/style.css (1)
1-36: Excellent CSS implementation with modern best practices.The styling demonstrates several strong patterns:
- Smart use of
:where()selector for specificity control- CSS custom properties with sensible fallbacks
- Proper accessibility styling for disabled states (
aria-disabled="true")- Clean hover interactions and cursor management
The conditional border styling approach is particularly elegant.
carousel/src/carousel/placeholder.js (2)
29-33: Smart auto-selection logic for single variations.The useEffect implementation correctly auto-selects when only one variation is available, providing a smooth user experience.
40-50: Proper accessibility implementation with documented ESLint exception.The ARIA roles and labels are correctly implemented, and the ESLint disable is properly justified with a clear explanation of the Safari+VoiceOver requirement.
carousel/src/carousel-nav/style.css (2)
45-47: Clever approach for directional icon support.The horizontal flip transformation for next button images provides an elegant solution for using the same icon asset for both previous and next buttons.
1-44: Well-implemented navigation button styling.The CSS demonstrates the same excellent patterns as the pagination component with proper use of CSS custom properties, accessibility considerations, and modern syntax.
carousel/src/carousel-pagination/block.json (1)
1-66: Well-structured WordPress block definition with proper API compliance.The block definition follows WordPress Block API v3 conventions correctly. The attribute definitions, supports configuration, and ancestor restrictions are properly implemented for a carousel pagination component.
carousel/src/carousel/view.css (2)
14-15: Excellent use of CSS Container Queries for responsive carousel behavior.The implementation of
container-name: carousel-trackandcontainer-type: inline-sizeenables responsive styling based on the carousel's container size rather than viewport, which is ideal for a reusable block component.
23-40: Well-implemented responsive image handling for gallery blocks.The differentiation between cropped (
is-cropped) and non-cropped gallery states with appropriate sizing andobject-fit: coverensures consistent visual presentation across different image aspect ratios.carousel/src/carousel-nav/block.json (1)
1-69: Consistent block definition following WordPress conventions.The navigation block definition mirrors the pagination block structure appropriately, with additional attributes for custom icon functionality. The ancestor restriction and attribute definitions are properly implemented.
carousel/src/carousel/common.js (1)
46-62: Excellent accessibility implementation with proper ARIA attributes.The conditional
aria-roledescriptionimplementation is well thought out - only adding it when a title is present avoids redundancy while maintaining accessibility standards.carousel/src/carousel/style.css (2)
26-44: Creative use of clip-path for overflow control.The implementation of different overflow behaviors using
clip-path: inset()is elegant and provides visual overflow control without affecting layout flow. The negative inset values effectively allow content to extend beyond the visible area while maintaining clipping on the desired sides.
55-66: Proper accessibility consideration with reduced motion preference.The
@media (prefers-reduced-motion: no-preference)query ensures animations are only applied for users who haven't requested reduced motion, following WCAG accessibility guidelines.carousel/src/carousel/variations.js (1)
1-140: Well-implemented block variations system!The implementation follows WordPress best practices with proper internationalization, conditional WooCommerce integration, and clean variation definitions. The use of
domReadyto conditionally add the products variation only when WooCommerce is available is a good approach.carousel/src/carousel/editor.css (1)
1-88: Solid CSS implementation with modern features!The editor styles are well-organized and make good use of modern CSS features like container queries, CSS grid, and custom properties. The responsive design considerations and accessibility-friendly hover states are implemented correctly.
carousel/src/carousel-pagination/common.js (1)
1-68: Excellent implementation of pagination utilities!The functions are well-designed with proper accessibility considerations. The
getHTMLAttributesfunction cleanly handles styling attributes with good filtering logic, and thepaginationButtonsfunction provides proper screen reader support with descriptive text.carousel/carousel.php (2)
57-89: Excellent implementation of modern WordPress block registration!The code properly implements the new WordPress 6.7+ block registration APIs with appropriate fallbacks for older versions. The conditional loading and performance optimizations using the blocks manifest approach are well done.
21-27: Good defensive programming with conditional class loading.The check for class existence before loading prevents conflicts when multiple WPCOMSP plugins are installed. This is a solid approach for plugin interoperability.
carousel/src/carousel-pagination/edit.js (3)
1-16: Good use of ESLint disable comments for experimental APIs.The imports are well-organized and the experimental WordPress APIs are properly marked with ESLint disable comments.
17-38: Well-structured component with proper context synchronization.The component correctly extracts attributes and context, and the
useEffecthook properly synchronizes the count attribute with the carousel's item count context.
39-87: Clean implementation of inspector controls and rendering.The component correctly implements color gradient controls and button size controls. The partial updates to
buttonColorsattribute preserve existing values properly.carousel/src/carousel-nav/edit.js (4)
1-29: Well-organized imports with media upload support.The imports properly include all necessary components including media upload functionality for custom icons. The attribute destructuring is comprehensive.
41-89: Consistent implementation of color controls.All four color settings properly spread the existing
buttonColorsobject when updating individual properties, maintaining data integrity.
90-178: Excellent implementation of custom icon upload.The custom icon feature is well-implemented with proper type restrictions, size-aware preview, and clear user instructions about icon flipping for the right button.
179-184: Clean render implementation.Good use of the
navigationButtonshelper function to maintain consistency between editor and frontend rendering.carousel/src/carousel/block.json (2)
1-31: Well-configured block metadata and supports.The block configuration appropriately disables HTML editing and layout switching while providing necessary spacing and color support options.
32-89: Comprehensive attribute definitions with proper constraints.The attributes are well-defined with appropriate types, defaults, and enum constraints. The context provision and file references are correctly configured.
carousel/src/carousel/edit.js (4)
1-77: Well-implemented helper functions.The recursive
findContentBlockandcreateBlockWithInnerBlocksfunctions properly handle nested block structures.
177-201: Proper attribute synchronization and block insertion.The
useEffectcorrectly syncs the item count, andselectVariationproperly handles the insertion of navigation and inner blocks.
203-334: Comprehensive and well-organized inspector controls.The controls provide all necessary carousel configuration options. Good UX consideration in automatically disabling pagination for infinite loop mode.
336-355: Clean conditional rendering implementation.The component appropriately shows a placeholder for initial setup and renders the full UI once inner blocks are present.
carousel/src/carousel-nav/common.js (1)
15-50: Well-implemented styling and accessibility attributes.The function properly constructs CSS custom properties for dynamic styling and includes appropriate ARIA attributes for accessibility.
carousel/src/carousel/view.js (1)
612-618: No action required:column-gapcorrectly reflects the CSSgapshorthandThe carousel’s CSS uses the
gapshorthand (e.g.gap: var(--wp--style--block-gap)), which under the hood sets bothrow-gapandcolumn-gap. When reading computed styles, queryingcolumn-gapon a horizontal (nowrap) flex container returns the intended horizontal spacing. The existinggetItemGapimplementation is therefore correct—no changes are needed.Likely an incorrect or invalid review comment.
| { | ||
| "name": "carousel", | ||
| "version": "0.1.0", | ||
| "description": "Display a horizontal series of content.", | ||
| "author": "The WordPress Contributors", | ||
| "license": "GPL-2.0-or-later", | ||
| "main": "build/index.js", | ||
| "scripts": { | ||
| "build": "wp-scripts build && wp-scripts build-blocks-manifest", | ||
| "fix:js": "wp-scripts lint-js --fix", | ||
| "format": "wp-scripts format", | ||
| "lint:css": "wp-scripts lint-style", | ||
| "lint:js": "wp-scripts lint-js", | ||
| "packages-update": "wp-scripts packages-update", | ||
| "plugin-zip": "wp-scripts plugin-zip", | ||
| "start": "wp-scripts start && wp-scripts build-blocks-manifest" | ||
| }, | ||
| "files": [ | ||
| "[^.]*" | ||
| ], | ||
| "devDependencies": { | ||
| "@wordpress/scripts": "^30.19.0", | ||
| "clsx": "^2.1.1" | ||
| }, | ||
| "dependencies": { | ||
| "@wordpress/icons": "^10.26.0" | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Package configuration looks solid with minor considerations.
The package.json follows WordPress block development standards well. The use of wp-scripts and proper licensing is appropriate.
Consider verifying the dependency versions are current and secure:
🌐 Web query:
What is the latest version of @wordpress/scripts and are there any security advisories for version 30.19.0?
💡 Result:
As of July 22, 2025, the latest version of @wordpress/scripts is 30.19.0. According to Snyk's vulnerability database, there are no direct vulnerabilities associated with this version. (security.snyk.io)
However, it's important to note that the latest non-vulnerable version is 30.19.1-next.719a03cbe.0. This indicates that while version 30.19.0 is currently free from known vulnerabilities, a newer version is available that may include additional fixes or improvements.
For the most up-to-date information on security advisories and to ensure your project remains secure, it's recommended to regularly check the Snyk Advisor page for @wordpress/scripts and the official WordPress releases page.
Citations:
Refine the “files” field in carousel/package.json
The @wordpress/scripts dependency (v30.19.0) is up-to-date and has no known vulnerabilities. However, the current files pattern
"files": [
"[^.]*"
]is overly broad and may bundle unintended assets. Please restrict it to only the directories and files you intend to publish. For example:
"files": [
- "[^.]*"
+ "build/",
+ "src/",
+ "package.json",
+ "README.md"
]This ensures only your compiled code, source, and key metadata are included in the package.
🤖 Prompt for AI Agents
In carousel/package.json from lines 1 to 28, the "files" field currently uses a
broad pattern "[^.]*" which may include unintended files in the package. To fix
this, replace the "files" array with a more specific list that explicitly
includes only the directories and files you want to publish, such as the build
output directory, source code directory, and essential metadata files like
README.md and package.json. This will ensure only the intended assets are
bundled in the package.
| === Carousel === | ||
| Contributors: The WordPress Contributors | ||
| Tags: block | ||
| Tested up to: 6.7 | ||
| Stable tag: 0.1.0 | ||
| License: GPL-2.0-or-later | ||
| License URI: https://www.gnu.org/licenses/gpl-2.0.html | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add mandatory header fields & clean up metadata
readme.txt parsed by WordPress.org requires Requires at least and Requires PHP headers; omitting them blocks upload.
While you’re touching this area, tighten the contributor/tag info.
-Contributors: The WordPress Contributors
-Tags: block
+Contributors: philcable # Use WP.org usernames
+Tags: block, carousel, slider
Tested up to: 6.7
Stable tag: 0.1.0
License: GPL-2.0-or-later
License URI: https://www.gnu.org/licenses/gpl-2.0.html
+# Mandatory for plugin repo compliance
+Requires at least: 6.4
+Requires PHP: 7.4📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| === Carousel === | |
| Contributors: The WordPress Contributors | |
| Tags: block | |
| Tested up to: 6.7 | |
| Stable tag: 0.1.0 | |
| License: GPL-2.0-or-later | |
| License URI: https://www.gnu.org/licenses/gpl-2.0.html | |
| === Carousel === | |
| Contributors: philcable # Use WP.org usernames | |
| Tags: block, carousel, slider | |
| Tested up to: 6.7 | |
| Stable tag: 0.1.0 | |
| License: GPL-2.0-or-later | |
| License URI: https://www.gnu.org/licenses/gpl-2.0.html | |
| # Mandatory for plugin repo compliance | |
| Requires at least: 6.4 | |
| Requires PHP: 7.4 |
🤖 Prompt for AI Agents
In carousel/readme.txt lines 1 to 8, add the mandatory header fields "Requires
at least" and "Requires PHP" with appropriate version values to comply with
WordPress.org requirements. Also, update the Contributors field to list specific
contributors instead of the generic "The WordPress Contributors" and refine the
Tags field to include more precise tags relevant to the plugin. Ensure the
metadata is clean and correctly formatted for WordPress parsing.
| const itemGap = style?.spacing?.blockGap; | ||
| if ( itemGap ) { | ||
| styles[ '--item-gap' ] = | ||
| itemGap.replace( | ||
| 'var:preset|spacing|', | ||
| 'var(--wp--preset--spacing--' | ||
| ) + ')'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential issue with block gap string replacement.
The string replacement approach for converting var:preset|spacing| to CSS custom properties could be fragile if the format changes or if the input doesn't match the expected pattern exactly.
Consider a more robust approach:
- const itemGap = style?.spacing?.blockGap;
- if ( itemGap ) {
- styles[ '--item-gap' ] =
- itemGap.replace(
- 'var:preset|spacing|',
- 'var(--wp--preset--spacing--'
- ) + ')';
- }
+ const itemGap = style?.spacing?.blockGap;
+ if ( itemGap ) {
+ if ( itemGap.startsWith( 'var:preset|spacing|' ) ) {
+ const spacingValue = itemGap.replace( 'var:preset|spacing|', '' );
+ styles[ '--item-gap' ] = `var(--wp--preset--spacing--${ spacingValue })`;
+ } else {
+ styles[ '--item-gap' ] = itemGap;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const itemGap = style?.spacing?.blockGap; | |
| if ( itemGap ) { | |
| styles[ '--item-gap' ] = | |
| itemGap.replace( | |
| 'var:preset|spacing|', | |
| 'var(--wp--preset--spacing--' | |
| ) + ')'; | |
| } | |
| const itemGap = style?.spacing?.blockGap; | |
| if ( itemGap ) { | |
| if ( itemGap.startsWith( 'var:preset|spacing|' ) ) { | |
| const spacingValue = itemGap.replace( 'var:preset|spacing|', '' ); | |
| styles[ '--item-gap' ] = `var(--wp--preset--spacing--${ spacingValue })`; | |
| } else { | |
| styles[ '--item-gap' ] = itemGap; | |
| } | |
| } |
🤖 Prompt for AI Agents
In carousel/src/carousel/common.js around lines 33 to 40, the current string
replacement for converting 'var:preset|spacing|' to CSS custom properties is
fragile and may fail if the input format changes. To fix this, implement a more
robust parsing method that validates the input pattern before replacement, such
as using a regular expression to match and transform the string safely, ensuring
it only replaces the intended pattern and handles unexpected formats gracefully.
Fix variable scoping in switch statement. Fix typo in JSDoc comment. Add null check for paginationButtons to prevent potential errors. Remove duplicate event listeners on pagination buttons. Handle non-numeric gap values to prevent NaN. Replace findLast with a more compatible solution (findLast is not supported in Safari < 15.4 and some other browsers).
The `not-allowed` cursor is kind of corny - this forgoes it in favor of being more consistent with the disabled nav button styles.
I think I'd avoided this because it gets a little weird when multiple slides are considered visible at once.
Implements a linked list, reorganizes, and renames functions in an attempt to improve scannability, readability, and maintainability.
Carousel block: refactor view script
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
carousel/src/carousel/edit.js (1)
106-165: Nice fix: block-scope variables inside switch cases.Wrapping each case in braces resolves Biome’s noSwitchDeclarations errors and avoids leakage across cases.
🧹 Nitpick comments (6)
carousel/src/carousel/edit.js (6)
112-161: Tighten query handling: remove unsupported perPage, avoid duplicate selects, and drop the ESLint disable.
- Combine the two select('core') calls.
- Don’t pass perPage to REST; translate to per_page and omit the original key.
- Remove the no-unused-vars suppression by skipping the key in destructuring.
- case 'core/query': - case 'woocommerce/product-collection': { - const { getTaxonomy } = select( 'core' ); - const { getEntityRecords } = select( 'core' ); + case 'core/query': + case 'woocommerce/product-collection': { + const { getTaxonomy, getEntityRecords } = select( 'core' ); const query = contentBlock.attributes.query || {}; - const postType = query?.postType || 'post'; + const postType = query?.postType || 'post'; + const { perPage, taxQuery: rawTaxQuery, ...restQuery } = query; // Remove falsey values from query parameters. const cleanQuery = Object.fromEntries( - Object.entries( { - ...query, - per_page: query.perPage || 10, - // eslint-disable-next-line no-unused-vars - } ).filter( ( [ _, value ] ) => { + Object.entries( { + ...restQuery, + per_page: perPage || 10, + } ).filter( ( [ , value ] ) => { if ( Array.isArray( value ) ) { return value.length > 0; } return ( value !== '' && value !== null && value !== undefined ); } ) ); // Handle taxonomy queries. - const taxQuery = query.taxQuery + const taxQuery = rawTaxQuery ? Object.fromEntries( - Object.entries( query.taxQuery ).map( + Object.entries( rawTaxQuery ).map( ( [ taxonomy, terms ] ) => { const taxonomyObj = getTaxonomy( taxonomy ); return [ taxonomyObj?.rest_base || taxonomy, terms, ]; } ) ) : {}; const records = getEntityRecords( 'postType', postType, { ...cleanQuery, ...taxQuery, _fields: [ 'id' ], } ); count = records?.length || 0; break; }
179-182: Avoid redundant attribute writes in effect.Guard setAttributes to prevent unnecessary updates/renders.
useEffect( () => { - setAttributes( { itemCount: Number( itemCount ) } ); - }, [ itemCount, setAttributes ] ); + const next = Number( itemCount ); + if ( attributes.itemCount !== next ) { + setAttributes( { itemCount: next } ); + } + }, [ itemCount, attributes.itemCount, setAttributes ] );
288-300: Validate and clamp animation speed.Prevent NaN/empty values and enforce min/max to keep CSS var sane.
<TextControl label={ __( 'Animation speed (in seconds)', 'carousel' ) } max="10" min="0.1" step="0.1" type="number" - onChange={ ( v ) => - setAttributes( { animationSpeed: Number( v ) } ) - } + onChange={ ( v ) => { + const n = Number( v ); + if ( Number.isFinite( n ) ) { + setAttributes( { + animationSpeed: Math.min( 10, Math.max( 0.1, n ) ), + } ); + } + } } value={ animationSpeed }
309-317: Validate track height input.Ensure numeric and non-negative to avoid invalid CSS.
<TextControl label={ __( 'Track height', 'carousel' ) } - onChange={ ( v ) => - setAttributes( { trackHeight: Number( v ) } ) - } + onChange={ ( v ) => { + const n = Number( v ); + if ( Number.isFinite( n ) ) { + setAttributes( { trackHeight: Math.max( 0, n ) } ); + } + } } type="number"
185-203: Prevent duplicate nav insertion on re-selection.If onSelect can re-fire, guard against inserting multiple wpcomsp/carousel-nav instances.
const selectVariation = ( nextVariation ) => { - insertBlock( - createBlock( 'wpcomsp/carousel-nav', {}, [] ), - undefined, - clientId - ); + // Insert nav only if not present. + const parent = wp.data.select( blockEditorStore ).getBlock( clientId ); + const hasNav = parent?.innerBlocks?.some( + ( b ) => b.name === 'wpcomsp/carousel-nav' + ); + if ( ! hasNav ) { + insertBlock( + createBlock( 'wpcomsp/carousel-nav', {}, [] ), + undefined, + clientId + ); + }Note: This uses wp.data.select; ensure it’s available or import { select as dataSelect } from '@wordpress/data' and use dataSelect( blockEditorStore ).
262-287: UX: clarify pagination behavior when infinite is selected.You’re auto-disabling pagination when animateEnd === 'infinite'. Consider adding help text or a notice to inform the user that pagination is disabled in infinite mode.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
carousel/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
carousel/src/carousel/block.json(1 hunks)carousel/src/carousel/edit.js(1 hunks)carousel/src/carousel/style.css(1 hunks)carousel/src/carousel/view.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- carousel/src/carousel/style.css
- carousel/src/carousel/block.json
🧰 Additional context used
🧬 Code graph analysis (2)
carousel/src/carousel/view.js (1)
carousel/src/carousel-pagination/common.js (1)
paginationButtons(53-68)
carousel/src/carousel/edit.js (2)
carousel/src/carousel/common.js (2)
attributes(15-26)getHTMLAttributes(14-63)carousel/src/carousel/save.js (1)
innerBlockProps(8-10)
🔇 Additional comments (10)
carousel/src/carousel/view.js (10)
1-1011: Comprehensive carousel implementation with solid architecture.This is a well-structured carousel implementation with proper separation of concerns, accessibility features, and multiple animation modes. The code demonstrates good understanding of DOM manipulation, event handling, and user experience considerations.
41-105: Well-designed circular linked list implementation.The
SlideNodeandCircularDoublyLinkedListclasses provide a clean abstraction for managing carousel slides with wrap-around behavior. The implementation correctly handles the circular nature of the data structure.
107-166: Robust drag controller with angle detection.The
DragControllerclass effectively handles drag/swipe navigation with proper angle threshold detection to distinguish between horizontal and vertical gestures. The implementation prevents unintended navigation during vertical scrolling.
183-231: Well-structured carousel instance class.The
CarouselInstanceclass provides a clean encapsulation of carousel state and behavior. The helper methods likehasTrackAndSlides(),isAnimating(), andgetTrackGap()improve code readability and maintainability.
363-379: Excellent accessibility implementation.The
setSlideAccessibilityfunction properly manages ARIA attributes and focus behavior for slides outside the viewport. This ensures screen readers and keyboard navigation work correctly with the carousel.
469-498: Dynamic pagination button creation.The
ensurePaginationButtonsfunction correctly ensures the number of pagination buttons matches the number of slides by dynamically creating missing buttons. The implementation properly updates the instance reference after DOM manipulation.
575-622: Comprehensive touch and mouse event handling.The drag navigation setup handles both mouse and touch events properly, including edge cases like
mouseleaveto ensure the drag state is reset when the cursor leaves the carousel area.
721-740: Sophisticated infinite navigation logic.The
navigateInfinitefunction implements complex DOM manipulation to maintain visual continuity during infinite scrolling. The use ofonCompletecallback ensures proper state management after animations.
934-980: Flexible carousel update mechanism.The
updateCarouselfunction provides a comprehensive API for carousel movement with optional animation, focus management, and completion callbacks. The implementation properly handles both animated and instant transitions.
991-1009: Clean infinite rotation commit logic.The
commitInfiniteRotationfunction elegantly reorders DOM elements and resets transforms to maintain infinite scrolling illusion. The implementation properly updates both the data structure and DOM state.
| const createBlockWithInnerBlocks = ( block ) => { | ||
| return createBlock( | ||
| block.name, | ||
| { | ||
| ...block.attributes, | ||
| metadata: block.metaData, | ||
| }, | ||
| block.innerBlocks?.map( createBlockWithInnerBlocks ) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm metadata vs metaData usage in variation blocks.
createBlock doesn’t typically accept a metadata attribute for saved content. Verify that block.metaData is intentional and that passing metadata here is required. Otherwise, drop it to avoid persisting an unsupported attribute.
Run to check occurrences and intent:
🏁 Script executed:
#!/bin/bash
# Find all uses of metaData/metadata in the repo to verify expected shape.
rg -nC2 -S '\bmetaData\b|\bmetadata\b'Length of output: 12886
Remove unsupported metadata assignment in createBlockWithInnerBlocks
block.metaData holds block-type metadata (from block.json/variations), not a declared block attribute—passing it here will serialize an unsupported “metadata” field. Remove the metadata: block.metaData entry.
🤖 Prompt for AI Agents
In carousel/src/carousel/edit.js around lines 67 to 76, remove the unsupported
metadata assignment inside createBlockWithInnerBlocks; specifically delete the
metadata: block.metaData property from the attributes spread so only declared
block attributes are passed when calling createBlock, ensuring inner block
attributes are preserved by mapping createBlockWithInnerBlocks over
block.innerBlocks.
This involves making a decision for the Product Collection block.
Carousel block: Implement a common selector for the carousel track
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
carousel/src/carousel/view.css (2)
3-12: Add explicit gap on the track.Relying only on --wp--style--block-gap may not affect non-block children (e.g., figures). Set gap to ensure consistent spacing.
.wp-block-wpcomsp-carousel-track { container-name: carousel-track; container-type: inline-size; display: flex; flex-wrap: nowrap; + gap: var(--item-gap); margin: 0; position: relative; - width: 100%; + inline-size: 100%; --wp--style--block-gap: var(--item-gap); }
21-30: Optional: make non‑cropped figures safer.If any themes add internal spacing, consider preventing overflow and centering content.
&:not(.is-cropped) figure.wp-block-image:not(#individual-image) { height: var(--track-height); width: auto; display: flex; + align-items: center; + overflow: hidden; img { height: 100%; object-fit: cover; + inline-size: auto; } }carousel/src/carousel/variations.js (2)
77-79: Fix property name: use metadata (not metaData).Block template labels expect metadata.name; current metaData will be ignored.
- metaData: { + metadata: { name: __( 'Carousel Track', 'carousel' ), },
8-27: Nit: simplify variation slugs.Variation name can be a simple slug (e.g., "images"), improving readability and avoiding “/” in IDs.
- name: 'wpcomsp/carousel-images', + name: 'images', @@ - name: 'wpcomsp/carousel-posts', + name: 'posts', @@ - name: 'wpcomsp/carousel-cards', + name: 'cards', @@ - name: 'wpcomsp/carousel-products', + name: 'products',Also applies to: 29-65, 67-123, 126-184
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
carousel/src/carousel/editor.css(1 hunks)carousel/src/carousel/style.css(1 hunks)carousel/src/carousel/variations.js(1 hunks)carousel/src/carousel/view.css(1 hunks)carousel/src/carousel/view.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- carousel/src/carousel/editor.css
- carousel/src/carousel/style.css
- carousel/src/carousel/view.js
🧰 Additional context used
🧬 Code graph analysis (1)
carousel/src/carousel/variations.js (1)
carousel/src/carousel/placeholder.js (1)
variations(20-23)
🔇 Additional comments (2)
carousel/src/carousel/variations.js (1)
6-123: Overall variations structure looks solid.Templates and attributes align with intended tracks and cover common use cases.
If index.js batches registration from this export, please confirm deduping with the product variation after adopting the registration fix above.
carousel/src/carousel/view.css (1)
1-32: Confirm nested CSS is compiled (view.css uses &-nesting)carousel/package.json uses @wordpress/scripts (wp-scripts), which runs PostCSS by default; I didn't find a project-level postcss.config.js — ensure carousel/src/carousel/view.css is included in the wp-scripts build (imported in an entry or block.json) or add a postcss.config.js that enables nesting (postcss-preset-env 'nesting-rules' or postcss-nested) so the & selectors are compiled. (developer.wordpress.org)
| name: 'woocommerce/product-image', | ||
| attributes: { | ||
| showSaleBadge: false, | ||
| imageSizing: 'thumbnail', | ||
| isDescendentOfQueryLoop: true, | ||
| }, | ||
| innerBlocks: [ | ||
| { | ||
| name: 'woocommerce/product-sale-badge', | ||
| attributes: { | ||
| align: 'right', | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| name: 'core/post-title', | ||
| attributes: { | ||
| isLink: true, | ||
| }, | ||
| }, | ||
| { | ||
| name: 'woocommerce/product-price', | ||
| attributes: { | ||
| isDescendentOfQueryLoop: true, | ||
| }, | ||
| }, | ||
| { | ||
| name: 'woocommerce/product-button', | ||
| attributes: { | ||
| isDescendentOfQueryLoop: true, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify attribute names for Woo blocks.
Double-check isDescendentOfQueryLoop, imageSizing, and nested product-sale-badge placement against current Woo Blocks API to avoid silent no‑ops.
🌐 Web query:
Confirm the correct attribute names and usage for:
- woocommerce/product-image: imageSizing, showSaleBadge, isDescendentOfQueryLoop
- woocommerce/product-price: isDescendentOfQueryLoop
- woocommerce/product-button: isDescendentOfQueryLoop
- Whether woocommerce/product-sale-badge is valid as an inner block of product-image.
💡 Result:
Short answer:
-
Confirmed attributes:
- woocommerce/product-image: imageSizing, showSaleBadge, isDescendentOfQueryLoop (also: aspectRatio, height, saleBadgeAlign, scale, showProductLink, width, productId, isDescendentOfSingleProductBlock). [1][2]
- woocommerce/product-price: isDescendentOfQueryLoop (also: isDescendentOfSingleProductBlock, isDescendentOfSingleProductTemplate, productId, textAlign). [1]
- woocommerce/product-button: isDescendentOfQueryLoop (also: isDescendentOfSingleProductBlock, productId, textAlign, width). [1]
-
Is woocommerce/product-sale-badge valid as an inner block of woocommerce/product-image?
- No — the Product Image block exposes its own showSaleBadge and saleBadgeAlign attributes and renders the sale badge internally (see block code). The separate woocommerce/product-sale-badge block exists and is usable in Single Product / product-template contexts, but it is not the intended inner-block mechanism for the Product Image (and there have been related issues/bugs reported when using the standalone sale-badge inside Single Product). [1][2][3]
Sources:
[1] WooCommerce Blocks reference / block documentation. (developer.woocommerce.com)
[2] WooCommerce code reference (product-image defaults / render_on_sale_badge). (woocommerce.github.io)
[3] GitHub issue discussing product-sale-badge rendering in Single Product context. (github.com)
Citations:
- 1: https://developer.woocommerce.com/docs/block-development/block-references/?utm_source=openai
- 2: https://woocommerce.github.io/code-reference/files/woocommerce-src-blocks-blocktypes-productimage.html?utm_source=openai
- 3: Product Sale Badge: the block isn't rendered on the frontend when used inside the Single Product Block woocommerce/woocommerce#57983
Attributes verified — remove nested woocommerce/product-sale-badge
- Confirmed: woocommerce/product-image accepts imageSizing, showSaleBadge, isDescendentOfQueryLoop; woocommerce/product-price and woocommerce/product-button accept isDescendentOfQueryLoop.
- Fix: Remove the nested 'woocommerce/product-sale-badge' inside product-image in carousel/src/carousel/variations.js (lines 144–176); the Product Image renders the sale badge itself — use showSaleBadge and saleBadgeAlign on product-image instead.
🤖 Prompt for AI Agents
In carousel/src/carousel/variations.js around lines 144–176, remove the nested
'woocommerce/product-sale-badge' object from the product-image innerBlocks and
instead set the sale badge via the product-image attributes: add saleBadgeAlign:
'right' and set showSaleBadge: true (or preserve existing showSaleBadge value if
you want it hidden); ensure innerBlocks for product-image no longer contains a
nested sale-badge entry.
| domReady( () => { | ||
| if ( getBlockType( 'woocommerce/product-collection' ) ) { | ||
| variations.push( productVariation ); | ||
| } | ||
| } ); |
There was a problem hiding this comment.
Product variation won’t register by just pushing to the array.
If index.js registers variations at import time, this push runs too late and the product variation never shows. Register it with the blocks API instead.
-import domReady from '@wordpress/dom-ready';
+import domReady from '@wordpress/dom-ready';
+import { registerBlockVariation } from '@wordpress/blocks';
@@
domReady( () => {
if ( getBlockType( 'woocommerce/product-collection' ) ) {
- variations.push( productVariation );
+ registerBlockVariation( 'wpcomsp/carousel', productVariation );
}
} );If you prefer not to register here, export a getter and call it where registration occurs:
-const variations = [ /* ... */ ];
+const baseVariations = [ /* ... */ ];
+export const getVariations = () => {
+ const list = [ ...baseVariations ];
+ if ( getBlockType( 'woocommerce/product-collection' ) ) list.push( productVariation );
+ return list;
+};
-export default variations;
+export default baseVariations;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| domReady( () => { | |
| if ( getBlockType( 'woocommerce/product-collection' ) ) { | |
| variations.push( productVariation ); | |
| } | |
| } ); | |
| import domReady from '@wordpress/dom-ready'; | |
| import { registerBlockVariation } from '@wordpress/blocks'; | |
| domReady( () => { | |
| if ( getBlockType( 'woocommerce/product-collection' ) ) { | |
| registerBlockVariation( 'wpcomsp/carousel', productVariation ); | |
| } | |
| } ); |
🤖 Prompt for AI Agents
carousel/src/carousel/variations.js around lines 186-190: the current domReady
callback pushes productVariation into the variations array too late (after
index.js already registers variations), so the variation never gets registered;
replace this late push by registering the variation via the WordPress blocks API
(wp.blocks.registerBlockVariation) at module load time or at an appropriate
initialization hook, or instead export a getter function for productVariation
and call that getter from the module that performs registration (index.js) so
registration happens synchronously when blocks are registered.
💡 Verification agent
🧩 Analysis chain
Confirm registration timing.
Ensure the code that registers variations runs after this module’s domReady callback, or adopt the direct registration fix to avoid timing issues.
🏁 Script executed:
#!/bin/bash
# Locate where variations are registered and verify timing.
rg -nP "registerBlockVariation\s*\(" -C2
rg -nP "from ['\"]\.\/variations['\"]" -C2
rg -nP "domReady\(" -C2Length of output: 1950
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== carousel/src/carousel/index.js ==="
sed -n '1,240p' carousel/src/carousel/index.js || true
echo
echo "=== carousel/src/carousel/variations.js ==="
sed -n '1,420p' carousel/src/carousel/variations.js || trueLength of output: 4322
Confirm registration timing.
carousel/src/carousel/index.js calls registerBlockType at module load with the imported variations array; carousel/src/carousel/variations.js pushes productVariation inside domReady (lines 186–190), so that push happens after registration and the product variation will not be included. Either run registerBlockType after domReady or register the variation directly (e.g., call registerBlockVariation inside the domReady callback).
🤖 Prompt for AI Agents
In carousel/src/carousel/variations.js around lines 186–190, the code pushes
productVariation into the exported variations array inside a domReady callback,
but registerBlockType in carousel/src/carousel/index.js runs at module load so
the push happens too late and the variation is never included; fix by either
moving block registration to run after domReady or, simpler, replace the push
with a direct registerBlockVariation call (or call registerBlockVariation within
the domReady callback) so the variation is registered at the correct time.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
carousel/src/carousel/view.js (3)
222-231: Read both column-gap and gap for robustness; coerce non‑numeric to 0.Some themes set gap, not column-gap. Fall back if column-gap is 0/NaN.
getTrackGap() { - return ( - parseFloat( - getComputedStyle( this.track ).getPropertyValue( - 'column-gap' - ) - ) || 0 - ); + const style = getComputedStyle( this.track ); + const col = parseFloat( style.getPropertyValue( 'column-gap' ) ); + if ( Number.isFinite( col ) && col > 0 ) return col; + const gap = parseFloat( style.getPropertyValue( 'gap' ) ); + return Number.isFinite( gap ) ? gap : 0; }
198-204: Remove unused dragState on instance.Dead state leftover from pre‑class implementation; it’s never read.
- this.dragState = { - startX: 0, - startY: 0, - isHorizontal: false, - dragging: false, - };
503-510: Avoid innerHTML for SR text; build nodes to reduce XSS surface.Even if values are numeric, prefer textContent.
- button.innerHTML = `<span class="screen-reader-text">Slide ${ i + 1 } of ${ slides.length }</span>`; + const sr = document.createElement( 'span' ); + sr.className = 'screen-reader-text'; + sr.textContent = `Slide ${ i + 1 } of ${ slides.length }`; + button.appendChild( sr );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
carousel/src/carousel/edit.js(1 hunks)carousel/src/carousel/variations.js(1 hunks)carousel/src/carousel/view.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
carousel/src/carousel/variations.js (1)
carousel/src/carousel/placeholder.js (1)
variations(20-23)
carousel/src/carousel/view.js (1)
carousel/src/carousel-pagination/common.js (1)
paginationButtons(53-68)
carousel/src/carousel/edit.js (3)
carousel/src/carousel-pagination/edit.js (2)
Edit(17-87)attributes(19-19)carousel/src/carousel/common.js (2)
attributes(15-26)getHTMLAttributes(14-63)carousel/src/carousel/save.js (1)
innerBlockProps(8-10)
🔇 Additional comments (3)
carousel/src/carousel/variations.js (2)
150-164: Remove nested woocommerce/product-sale-badge; set on product-image via attributes.The Product Image block renders the sale badge itself; the nested sale-badge is not a valid inner block here.
{ name: 'woocommerce/product-image', attributes: { - showSaleBadge: false, + showSaleBadge: true, + saleBadgeAlign: 'right', imageSizing: 'thumbnail', isDescendentOfQueryLoop: true, }, - innerBlocks: [ - { - name: 'woocommerce/product-sale-badge', - attributes: { - align: 'right', - }, - }, - ], },
195-199: Register the product variation instead of pushing after registration has happened.Pushing inside domReady runs too late; the variation won’t appear. Use the blocks API to register it.
-import { getBlockType } from '@wordpress/blocks'; +import { getBlockType, registerBlockVariation } from '@wordpress/blocks'; @@ domReady( () => { if ( getBlockType( 'woocommerce/product-collection' ) ) { - variations.push( productVariation ); + registerBlockVariation( 'wpcomsp/carousel', productVariation ); } } );carousel/src/carousel/edit.js (1)
67-76: Don’t pass metadata via createBlock; it serializes an unsupported field.Drop the
metadata: block.metaDataspread; keep attributes only.const createBlockWithInnerBlocks = ( block ) => { return createBlock( block.name, { - ...block.attributes, - metadata: block.metaData, + ...block.attributes, }, block.innerBlocks?.map( createBlockWithInnerBlocks ) ); };
| carousel.addEventListener( 'mousedown', ( e ) => { | ||
| e.preventDefault(); // Prevent text selection. | ||
| dragController.onStart( e.clientX, e.clientY ); | ||
| } ); | ||
|
|
||
| carousel.addEventListener( 'mousemove', ( e ) => { | ||
| dragController.onMove( e.clientX, e.clientY, e ); | ||
| } ); | ||
|
|
||
| carousel.addEventListener( 'touchstart', ( e ) => { | ||
| dragController.onStart( | ||
| e.touches[ 0 ].clientX, | ||
| e.touches[ 0 ].clientY | ||
| ); | ||
| } ); | ||
|
|
||
| carousel.addEventListener( 'touchmove', ( e ) => { | ||
| dragController.onMove( | ||
| e.touches[ 0 ].clientX, | ||
| e.touches[ 0 ].clientY, | ||
| e | ||
| ); | ||
| } ); | ||
|
|
||
| [ 'touchend', 'mouseup', 'mouseleave' ].forEach( ( eventType ) => { | ||
| carousel.addEventListener( eventType, () => | ||
| dragController.onEnd() | ||
| ); | ||
| } ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Don’t block clicks/focus; limit mousedown preventDefault to non‑interactive targets.
Preventing default on every mousedown can suppress focus/click on links, buttons, etc. Guard it.
- carousel.addEventListener( 'mousedown', ( e ) => {
- e.preventDefault(); // Prevent text selection.
- dragController.onStart( e.clientX, e.clientY );
- } );
+ carousel.addEventListener( 'mousedown', ( e ) => {
+ // Allow native interaction inside slides.
+ if ( e.target.closest( SELECTORS.FOCUSABLE ) ) {
+ return;
+ }
+ e.preventDefault(); // Prevent text selection/drag.
+ dragController.onStart( e.clientX, e.clientY );
+ } );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| carousel.addEventListener( 'mousedown', ( e ) => { | |
| e.preventDefault(); // Prevent text selection. | |
| dragController.onStart( e.clientX, e.clientY ); | |
| } ); | |
| carousel.addEventListener( 'mousemove', ( e ) => { | |
| dragController.onMove( e.clientX, e.clientY, e ); | |
| } ); | |
| carousel.addEventListener( 'touchstart', ( e ) => { | |
| dragController.onStart( | |
| e.touches[ 0 ].clientX, | |
| e.touches[ 0 ].clientY | |
| ); | |
| } ); | |
| carousel.addEventListener( 'touchmove', ( e ) => { | |
| dragController.onMove( | |
| e.touches[ 0 ].clientX, | |
| e.touches[ 0 ].clientY, | |
| e | |
| ); | |
| } ); | |
| [ 'touchend', 'mouseup', 'mouseleave' ].forEach( ( eventType ) => { | |
| carousel.addEventListener( eventType, () => | |
| dragController.onEnd() | |
| ); | |
| } ); | |
| // Only prevent text selection/drag for non-interactive areas. | |
| carousel.addEventListener( 'mousedown', ( e ) => { | |
| // Allow native interaction inside slides. | |
| if ( e.target.closest( SELECTORS.FOCUSABLE ) ) { | |
| return; | |
| } | |
| e.preventDefault(); // Prevent text selection/drag. | |
| dragController.onStart( e.clientX, e.clientY ); | |
| } ); | |
| carousel.addEventListener( 'mousemove', ( e ) => { | |
| dragController.onMove( e.clientX, e.clientY, e ); | |
| } ); | |
| carousel.addEventListener( 'touchstart', ( e ) => { | |
| dragController.onStart( | |
| e.touches[ 0 ].clientX, | |
| e.touches[ 0 ].clientY | |
| ); | |
| } ); | |
| carousel.addEventListener( 'touchmove', ( e ) => { | |
| dragController.onMove( | |
| e.touches[ 0 ].clientX, | |
| e.touches[ 0 ].clientY, | |
| e | |
| ); | |
| } ); | |
| [ 'touchend', 'mouseup', 'mouseleave' ].forEach( ( eventType ) => { | |
| carousel.addEventListener( eventType, () => | |
| dragController.onEnd() | |
| ); | |
| } ); |
🤖 Prompt for AI Agents
In carousel/src/carousel/view.js around lines 611 to 639, the mousedown handler
currently calls e.preventDefault() unconditionally which blocks clicks/focus on
interactive elements; change the handler to first check whether the event target
or any of its ancestors is an interactive element (e.g. a, button, input,
textarea, select, [contenteditable], or elements with role="button" or
role="link", and skip disabled controls), and only call e.preventDefault() and
start the drag when no such interactive ancestor exists; otherwise simply allow
the event to proceed (but still call dragController.onStart only when
appropriate).
| carousel.addEventListener( 'touchmove', ( e ) => { | ||
| dragController.onMove( | ||
| e.touches[ 0 ].clientX, | ||
| e.touches[ 0 ].clientY, | ||
| e | ||
| ); | ||
| } ); |
There was a problem hiding this comment.
Enable preventDefault() on touchmove by using non‑passive listeners.
Without { passive: false }, preventDefault is ignored on many mobile browsers, breaking swipe.
- carousel.addEventListener( 'touchmove', ( e ) => {
+ carousel.addEventListener( 'touchmove', ( e ) => {
dragController.onMove(
e.touches[ 0 ].clientX,
e.touches[ 0 ].clientY,
e
);
- } );
+ }, { passive: false } );
+ // Optional: also mark touchstart as non-passive if you ever call preventDefault there.
+ // carousel.addEventListener('touchstart', handler, { passive: false });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| carousel.addEventListener( 'touchmove', ( e ) => { | |
| dragController.onMove( | |
| e.touches[ 0 ].clientX, | |
| e.touches[ 0 ].clientY, | |
| e | |
| ); | |
| } ); | |
| carousel.addEventListener( 'touchmove', ( e ) => { | |
| dragController.onMove( | |
| e.touches[ 0 ].clientX, | |
| e.touches[ 0 ].clientY, | |
| e | |
| ); | |
| }, { passive: false } ); | |
| // Optional: also mark touchstart as non-passive if you ever call preventDefault there. | |
| // carousel.addEventListener('touchstart', handler, { passive: false }); |
🤖 Prompt for AI Agents
In carousel/src/carousel/view.js around lines 627 to 633, the touchmove listener
is added without options so calling e.preventDefault() will be ignored on many
mobile browsers; update the addEventListener call to register the touchmove
handler as a non-passive listener (pass { passive: false }) and ensure the
handler calls e.preventDefault() where appropriate to stop the default touch
behavior during swipe.
| function updateCarousel( | ||
| instance, | ||
| offset, | ||
| { animate = true, updateFocus = false, onComplete = null } = {} | ||
| ) { | ||
| const { carousel, slides } = instance; | ||
|
|
||
| if ( animate ) { | ||
| carousel.classList.add( CLASSES.ANIMATING ); | ||
| } | ||
|
|
||
| slides.forEach( ( slide ) => { | ||
| slide.style.transform = `translate3d(${ offset }px, 0, 0)`; | ||
| } ); | ||
|
|
||
| const handleCompletion = () => { | ||
| if ( onComplete ) { | ||
| onComplete(); | ||
| } | ||
|
|
||
| if ( updateFocus ) { | ||
| // Ensure the target slide can receive focus even if its visibility | ||
| // is not yet updated by the IntersectionObserver. | ||
| setSlideAccessibility( instance.currentNode.slide, true ); | ||
|
|
||
| // Defer focus reset by one frame for good measure. | ||
| requestAnimationFrame( () => { | ||
| instance.currentNode.slide | ||
| .querySelector( SELECTORS.FOCUSABLE ) | ||
| ?.focus(); | ||
| } ); | ||
| } | ||
| }; | ||
|
|
||
| const onTransitionEnd = () => { | ||
| carousel.classList.remove( CLASSES.ANIMATING ); | ||
| handleCompletion(); | ||
| }; | ||
|
|
||
| if ( animate ) { | ||
| slides[ 0 ].addEventListener( 'transitionend', onTransitionEnd, { | ||
| once: true, | ||
| } ); | ||
| } else { | ||
| handleCompletion(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add a transition fallback to avoid getting stuck in is‑animating (e.g., reduced motion).
If transitions are disabled or duration is zero, transitionend won’t fire and controls remain disabled.
if ( animate ) {
carousel.classList.add( CLASSES.ANIMATING );
}
@@
- const onTransitionEnd = () => {
+ let timeoutId;
+ const onTransitionEnd = () => {
carousel.classList.remove( CLASSES.ANIMATING );
+ if ( timeoutId ) clearTimeout( timeoutId );
handleCompletion();
};
if ( animate ) {
- slides[ 0 ].addEventListener( 'transitionend', onTransitionEnd, {
- once: true,
- } );
+ slides[ 0 ].addEventListener( 'transitionend', onTransitionEnd, { once: true } );
+ // Fallback timer based on computed transition-duration.
+ const durations = getComputedStyle( slides[ 0 ] ).transitionDuration.split(',');
+ const ms = Math.max(
+ ...durations.map( d => {
+ const v = parseFloat( d ) || 0;
+ return d.trim().endsWith('ms') ? v : v * 1000;
+ } )
+ );
+ timeoutId = setTimeout( onTransitionEnd, (ms || 0) + 50 );
} else {
handleCompletion();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function updateCarousel( | |
| instance, | |
| offset, | |
| { animate = true, updateFocus = false, onComplete = null } = {} | |
| ) { | |
| const { carousel, slides } = instance; | |
| if ( animate ) { | |
| carousel.classList.add( CLASSES.ANIMATING ); | |
| } | |
| slides.forEach( ( slide ) => { | |
| slide.style.transform = `translate3d(${ offset }px, 0, 0)`; | |
| } ); | |
| const handleCompletion = () => { | |
| if ( onComplete ) { | |
| onComplete(); | |
| } | |
| if ( updateFocus ) { | |
| // Ensure the target slide can receive focus even if its visibility | |
| // is not yet updated by the IntersectionObserver. | |
| setSlideAccessibility( instance.currentNode.slide, true ); | |
| // Defer focus reset by one frame for good measure. | |
| requestAnimationFrame( () => { | |
| instance.currentNode.slide | |
| .querySelector( SELECTORS.FOCUSABLE ) | |
| ?.focus(); | |
| } ); | |
| } | |
| }; | |
| const onTransitionEnd = () => { | |
| carousel.classList.remove( CLASSES.ANIMATING ); | |
| handleCompletion(); | |
| }; | |
| if ( animate ) { | |
| slides[ 0 ].addEventListener( 'transitionend', onTransitionEnd, { | |
| once: true, | |
| } ); | |
| } else { | |
| handleCompletion(); | |
| } | |
| } | |
| function updateCarousel( | |
| instance, | |
| offset, | |
| { animate = true, updateFocus = false, onComplete = null } = {} | |
| ) { | |
| const { carousel, slides } = instance; | |
| if ( animate ) { | |
| carousel.classList.add( CLASSES.ANIMATING ); | |
| } | |
| slides.forEach( ( slide ) => { | |
| slide.style.transform = `translate3d(${ offset }px, 0, 0)`; | |
| } ); | |
| const handleCompletion = () => { | |
| if ( onComplete ) { | |
| onComplete(); | |
| } | |
| if ( updateFocus ) { | |
| // Ensure the target slide can receive focus even if its visibility | |
| // is not yet updated by the IntersectionObserver. | |
| setSlideAccessibility( instance.currentNode.slide, true ); | |
| // Defer focus reset by one frame for good measure. | |
| requestAnimationFrame( () => { | |
| instance.currentNode.slide | |
| .querySelector( SELECTORS.FOCUSABLE ) | |
| ?.focus(); | |
| } ); | |
| } | |
| }; | |
| let timeoutId; | |
| const onTransitionEnd = () => { | |
| carousel.classList.remove( CLASSES.ANIMATING ); | |
| if ( timeoutId ) { | |
| clearTimeout( timeoutId ); | |
| } | |
| handleCompletion(); | |
| }; | |
| if ( animate ) { | |
| slides[ 0 ].addEventListener( 'transitionend', onTransitionEnd, { once: true } ); | |
| // Fallback timer based on computed transition-duration. | |
| const durations = getComputedStyle( slides[ 0 ] ).transitionDuration.split(','); | |
| const ms = Math.max( | |
| ...durations.map( d => { | |
| const v = parseFloat( d ) || 0; | |
| return d.trim().endsWith('ms') ? v : v * 1000; | |
| } ) | |
| ); | |
| timeoutId = setTimeout( onTransitionEnd, (ms || 0) + 50 ); | |
| } else { | |
| handleCompletion(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In carousel/src/carousel/view.js around lines 952 to 998, the code relies solely
on the 'transitionend' event to clear the ANIMATING state which can never fire
when transitions are disabled or have zero duration (or the user
prefers-reduced-motion), causing controls to remain stuck; fix by detecting
whether a transition will actually occur (check computed
transition-duration/transition-delay or prefers-reduced-motion) and if no
transition is expected, call the same completion logic immediately (or after a
tiny requestAnimationFrame) instead of waiting for 'transitionend', otherwise
use a safety timeout equal to the parsed transition duration plus a small buffer
so handleCompletion/onTransitionEnd always runs and the ANIMATING class is
removed.
3 participants
Summary by CodeRabbit