Refactor carousel block deprecations and ariaLabelPattern - #188
Refactor carousel block deprecations and ariaLabelPattern#188milindmore22 wants to merge 4 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.
Refactors Carousel block deprecations by centralizing shared schema/support definitions, adding a v2.1.0 deprecated save implementation, and standardizing ariaLabelPattern as a plain string for saved output.
Changes:
- Added
SaveV210and updated thedeprecatedarray ordering to include it as the latest deprecated save. - Introduced
sharedAttributes/sharedSupportsto reduce duplication across deprecated entries. - Added unit tests for deprecated entries and exported deprecated save functions for testability.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/blocks/carousel/save.tsx | Removes translation call from ariaLabelPattern in the current save output. |
| src/blocks/carousel/deprecated.tsx | Adds SaveV210, centralizes deprecated schema/supports, and removes translation call from deprecated ariaLabelPattern. |
| src/blocks/carousel/tests/deprecated.test.tsx | Adds tests validating the deprecated array structure and SaveV210 context output. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…t translation of ariaLabelPattern for consistent markup
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Suppressed comments (2)
src/blocks/carousel/save.tsx:66
- This makes
ariaLabelPatternpermanently English in saved output. Because this value is intended for ARIA label text, hard-coding it prevents localization for non-English sites (accessibility regression). A better approach is to keep saved markup stable while still localizing at runtime (e.g., store a stable message key/identifier in saved context and translate in the interactive script), rather than persisting an English string.
// Un-translated pattern to keep saved block markup consistent across translations
ariaLabelPattern: 'Go to slide %d',
src/blocks/carousel/tests/deprecated.test.tsx:59
- This test is brittle because it hard-codes the total number of deprecated entries. Adding a future deprecation (which is expected over time) will fail the test even if ordering and behavior remain correct. Consider asserting the relative ordering of these known entries (e.g., first entry is
SaveV210, andSaveV203appears beforeSaveV200) without asserting the exact array length.
it( 'should export a deprecated array with three deprecation entries', () => {
expect( Array.isArray( deprecated ) ).toBe( true );
expect( deprecated ).toHaveLength( 3 );
expect( deprecated[ 0 ].save ).toBe( SaveV210 );
expect( deprecated[ 1 ].save ).toBe( SaveV203 );
expect( deprecated[ 2 ].save ).toBe( SaveV200 );
} );
…ontend context serialization
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (4)
src/blocks/carousel/types.ts:31
carouselIdis optional inCarouselAttributes, butblock.jsondefines it as a string attribute with a default (\"\"), and the save implementations treat it as a string. Consider making thiscarouselId: string;to keep TS types aligned with the block schema and reduce the need for runtime fallback logic.
autoScrollStopOnInteraction: boolean;
autoScrollStopOnMouseEnter: boolean;
useTabs: boolean;
carouselId?: string;
};
src/blocks/carousel/save.tsx:90
- This fallback is redundant given
carouselIdis already defaulted during destructuring (carouselId = ''). If you still want a defensive fallback, prefer nullish coalescing (carouselId ?? '') to avoid treating other falsy values as empty (and otherwise just passcarouselId).
carouselId: carouselId || '',
src/blocks/carousel/deprecated.tsx:276
- The comment on
ariaLabelPatternindicates the goal is locale-independent saved markup, butcountLabelPatternandannouncementPatternare still translated duringsave, which keepsdata-wp-contextlocale-dependent. To fully avoid validation drift across locales, consider also making these un-translated insave(and translating at runtime in JS), or revising the comment/rationale so it’s not misleading.
// Un-translated pattern to keep saved block markup consistent across translations
ariaLabelPattern: 'Go to slide %d',
/* translators: {{currentSlide}}: current slide number, {{totalSlides}}: total slide count. */
countLabelPattern: __(
'Slide {{currentSlide}} of {{totalSlides}}',
'rt-carousel',
),
announcement: '',
shouldAnnounce: false,
/* translators: {{currentSlide}}: current slide number, {{totalSlides}}: total slide count. */
announcementPattern: __(
'Slide {{currentSlide}} of {{totalSlides}}',
'rt-carousel',
),
src/blocks/carousel/block.json:131
- This PR introduces a new persisted attribute (
carouselId) in the block schema, and it’s now serialized intodata-wp-contextinsave.tsx. The PR description focuses on deprecated refactors +ariaLabelPattern; consider explicitly calling out this schema/API addition (even if non-breaking) so downstream consumers understand the new attribute.
"useTabs": {
"type": "boolean",
"default": false
},
"carouselId": {
"type": "string",
"default": ""
}
Summary
This pull request refactors the deprecated versions of the Carousel block to improve maintainability and test coverage. It introduces a shared attributes and supports schema for all deprecated entries, adds a new
SaveV210function for the v2.1.0 save implementation, and provides comprehensive tests for the deprecations. Additionally, it removes the translation function from theariaLabelPatternfield in all save implementations.Refactoring and Code Maintenance
sharedAttributesandsharedSupportsobject to unify the schema for all deprecated Carousel block versions, reducing duplication and making future updates easier. [1] [2]SaveV210function to represent the v2.1.0 save logic, and updated thedeprecatedarray to include this version as the latest deprecation entry. [1] [2]SaveV200,SaveV203, andSaveV210for use in tests and other modules.Testing Improvements
deprecated.test.tsxto verify the structure and behavior of the deprecated Carousel block versions, including attribute schemas and context output.Internationalization
__) from theariaLabelPatternfield in allsaveimplementations for both current and deprecated Carousel block versions, standardizing it as a plain string. [1] [2] [3]Type of change
Related issue(s)
N/A
What changed
Breaking changes
Does this introduce a breaking change? If yes, describe the impact and migration path below.
Testing
Describe how this was tested.
Test details:
Screenshots / recordings
If applicable, add screenshots or short recordings.
Checklist