fix: Ensure tabs are properly saved and backfill deprecations - #188
Open
milindmore22 wants to merge 16 commits into
Open
fix: Ensure tabs are properly saved and backfill deprecations#188milindmore22 wants to merge 16 commits into
milindmore22 wants to merge 16 commits into
Conversation
Contributor
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
Contributor
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
Contributor
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": ""
}
justlevine
reviewed
Aug 10, 2026
justlevine
reviewed
Aug 10, 2026
justlevine
reviewed
Aug 10, 2026
justlevine
reviewed
Aug 10, 2026
…ated block registrations
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
src/blocks/carousel/types.ts:31
carouselIdis defined with a default of\"\"inblock.json(and in the deprecated attribute schema), so it will be present as a string at runtime. Making it optional inCarouselAttributescan create unnecessaryundefinedhandling and mismatch the block schema; consider making it a requiredstringinstead (or ensure all attribute access sites treat it as optional consistently).
autoScrollStopOnInteraction: boolean;
autoScrollStopOnMouseEnter: boolean;
useTabs: boolean;
carouselId?: string;
};
src/blocks/carousel/tests/deprecated.test.tsx:71
- The new tests primarily validate array structure and
SaveV210output; they don’t exerciseSaveV200/SaveV203rendering or key serialization behaviors (notably theariaLabelPatternchange described in the PR summary). To better cover the refactor and prevent regressions, add assertions thatSaveV200/SaveV203produce the expected wrapper props/context (and explicitly validateariaLabelPatternis a plain string for each save implementation).
describe( 'Carousel Deprecations', () => {
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 );
} );
it( 'should include all attributes in shared attributes schema across deprecation entries', () => {
deprecated.forEach( ( entry ) => {
expect( entry.attributes ).toHaveProperty( 'transition' );
expect( entry.attributes ).toHaveProperty( 'lazyLoadImages' );
expect( entry.attributes ).toHaveProperty( 'autoScroll' );
expect( entry.attributes ).toHaveProperty( 'useTabs' );
} );
} );
describe( 'SaveV210', () => {
it( 'renders correctly without transition but with autoScroll and useTabs in raw JSON context', () => {
const { container } = render( <SaveV210 attributes={ mockAttributes } /> );
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/blocks/carousel/types.ts:31
carouselIdis typed as optional inCarouselAttributes, butblock.json(andsharedAttributesin deprecations) define a default of\"\", which makes it effectively always present at runtime. To keep the public type aligned with the block schema, consider makingcarouselIdnon-optional (carouselId: string) or removing the default in the schema ifundefinedis a meaningful state.
autoScrollStopOnInteraction: boolean;
autoScrollStopOnMouseEnter: boolean;
useTabs: boolean;
carouselId?: string;
};
src/blocks/carousel/deprecated.tsx:288
carouselIdis now part of the attributes schema (including deprecated schemas), butSaveV210hard-codescontext.carouselIdto an empty string and does not read fromattributes. IfcarouselIdis intended to be a persisted attribute, setcontext.carouselIdfromattributes.carouselId(with an empty-string fallback). If it is strictly runtime-only, consider removing it from the block/deprecated attribute schemas to avoid a confusing attribute that is never honored by save output.
}
: false,
useTabs,
carouselId: '', // Set at runtime by initCarousel in view.ts
};
src/blocks/carousel/deprecated.tsx:385
- The new tests exercise
SaveV210, but the refactor also changes the deprecated wiring forSaveV203andSaveV200(they now share the same attribute/support objects). Add targeted assertions forSaveV203andSaveV200saves as well (e.g., verifyingdata-wp-contextserialization still matches expectations, including theariaLabelPatternstring behavior), so regressions in older deprecations are caught.
const deprecated = [
{
attributes: sharedAttributes,
supports: sharedSupports,
save: SaveV210,
},
{
attributes: sharedAttributes,
supports: sharedSupports,
save: SaveV203,
},
{
attributes: sharedAttributes,
supports: sharedSupports,
save: SaveV200,
},
…e functions from array export
justlevine
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
src/blocks/carousel/deprecated.tsx:151
- The PR description says the translation function was removed from
ariaLabelPattern“in all save implementations”, butSaveV201still wrapsariaLabelPatternwith__(). To match the stated intent (and the shipped-markup tests’ approach), setariaLabelPatternto the plain string value here (and any other deprecated saves where it’s still translated).
/* translators: %d: slide number */
ariaLabelPattern: __( 'Go to slide %d', 'rt-carousel' ),
announcement: '',
shouldAnnounce: false,
/* translators: {{currentSlide}}: current slide number, {{totalSlides}}: total slide count. */
announcementPattern: __(
'Slide {{currentSlide}} of {{totalSlides}}',
'rt-carousel',
),
tests/php/stubs.php:122
- In the stub,
remove_attribute()directly indexes$this->elements[ $this->cursor ]without the defensive?? nullbehavior you added viacurrent_element(). Ifremove_attribute()is called when the cursor isn’t positioned on a valid element, this will trigger an undefined offset / property access error in tests. Prefer usingcurrent_element()(and returningfalsewhen it’snull) to keep stub behavior predictable and consistent with the new helper.
public function remove_attribute( string $name ): bool {
$element = $this->elements[ $this->cursor ];
if ( ! $element->hasAttribute( $name ) ) {
return false;
}
$element->removeAttribute( $name );
return true;
}
src/blocks/carousel/slide/tests/save.test.tsx:30
- These tests assert exact
innerHTMLstrings, which is brittle (attribute ordering/serialization can change across React/test-renderer updates without changing semantics). To make the tests more resilient while still protecting “no markup drift”, consider asserting via DOM queries/attribute checks (e.g., role, classes, and presence/absence of specific attributes) rather than full-string equality.
expect( renderSave( {} ) ).toBe( SHIPPED_MARKUP );
src/blocks/carousel/slide/tests/save.test.tsx:39
- These tests assert exact
innerHTMLstrings, which is brittle (attribute ordering/serialization can change across React/test-renderer updates without changing semantics). To make the tests more resilient while still protecting “no markup drift”, consider asserting via DOM queries/attribute checks (e.g., role, classes, and presence/absence of specific attributes) rather than full-string equality.
expect( renderSave( { verticalAlignment: 'center' } ) ).toBe(
SHIPPED_MARKUP.replace(
'class="embla__slide"',
'class="embla__slide is-vertically-aligned-center"',
),
);
Comment on lines
+415
to
+419
| $processor = \WP_HTML_Processor::create_fragment( $block_content ); | ||
|
|
||
| if ( null === $processor ) { | ||
| return $block_content; | ||
| } |
Collaborator
There was a problem hiding this comment.
the minimum WP version is 6.6, it's the preexisting conditional checks that are AI slop
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.
Suppressed comments (4)
src/blocks/carousel/deprecated.tsx:144
- These deprecated save implementations embed translated strings into the saved HTML context (
data-wp-context). That makes the output locale-dependent and can cause block invalidation / mismatches across locales, and it also conflicts with the PR description stating the translation function was removed fromariaLabelPatternin save implementations. Use plain, non-translated strings forariaLabelPattern(and consider doing the same for the other context patterns if the goal is deterministic saved markup), and remove the associatedtranslators:comments if translation is no longer used here.
/* translators: %d: slide number */
ariaLabelPattern: __( 'Go to slide %d', 'rt-carousel' ),
src/blocks/carousel/deprecated.tsx:151
- These deprecated save implementations embed translated strings into the saved HTML context (
data-wp-context). That makes the output locale-dependent and can cause block invalidation / mismatches across locales, and it also conflicts with the PR description stating the translation function was removed fromariaLabelPatternin save implementations. Use plain, non-translated strings forariaLabelPattern(and consider doing the same for the other context patterns if the goal is deterministic saved markup), and remove the associatedtranslators:comments if translation is no longer used here.
announcementPattern: __(
'Slide {{currentSlide}} of {{totalSlides}}',
'rt-carousel',
),
tests/php/stubs.php:122
- In the stub,
remove_attribute()directly indexes$this->elements[ $this->cursor ]without a bounds check. If the cursor is not positioned on an element (or test code callsremove_attribute()at the wrong time), this can raise an undefined index / fatal in PHP tests. Align the stub with safer behavior by using the newcurrent_element()helper (or a null coalescing guard) and returningfalsewhen there is no current element.
public function remove_attribute( string $name ): bool {
$element = $this->elements[ $this->cursor ];
if ( ! $element->hasAttribute( $name ) ) {
return false;
}
$element->removeAttribute( $name );
return true;
}
src/blocks/carousel/deprecated.tsx:363
- The PR description mentions adding a
SaveV210deprecation entry (v2.1.0) and exportingSaveV200,SaveV203, andSaveV210, but in this diff hunk the deprecated list only includesSaveV203,SaveV201, andSaveV200and shows noSaveV210. Either add the v2.1.0 entry here (if it’s intended) or update the PR description to match what’s actually being changed.
const deprecated = [
{
attributes: sharedAttributes,
supports: sharedSupports,
save: SaveV203,
},
{
attributes: sharedAttributes,
supports: sharedSupports,
save: SaveV201,
},
{
attributes: sharedAttributes,
supports: sharedSupports,
save: SaveV200,
},
];
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated no new comments.
Suppressed comments (5)
tests/php/stubs.php:122
remove_attribute()reads$this->elements[ $this->cursor ]without guarding for an unset/invalid cursor, which can emit PHP notices in tests (and differs from realWP_HTML_Tag_Processorbehavior, which should fail gracefully when not parked on a tag). Consider usingcurrent_element()and returningfalsewhen it isnull.
public function remove_attribute( string $name ): bool {
$element = $this->elements[ $this->cursor ];
if ( ! $element->hasAttribute( $name ) ) {
return false;
}
$element->removeAttribute( $name );
return true;
}
inc/Plugin.php:341
handle_block_markup()can result in two full HTML-processor passes in tabs mode (one inmark_query_loop_slides(), then another inmark_tab_panels()). Since these run on every render of the carousel block, this adds avoidable CPU cost for large carousels/pages. A concrete improvement would be to refactor so a singleWP_HTML_Processorwalk performs both transformations (or somark_query_loop_slides()can accept/reuse an existing processor).
public function handle_block_markup( string $block_content, array $parsed_block ): string {
// Handle before marking tab panels so that Query Loop slides get the active-slide directives first.
$block_content = $this->mark_query_loop_slides( $block_content );
return $this->mark_tab_panels( $block_content, $parsed_block );
}
src/blocks/carousel/deprecated.tsx:144
- PR description says the translation function was removed from
ariaLabelPatternin all save implementations (including deprecated ones), but the newSaveV201still uses__()forariaLabelPattern/announcementPattern. If the intent is not to change deprecated output (often necessary to match historically localized saved markup), the description should be updated to clarify that only current saves were de-i18n’d while deprecations retain__()for compatibility; otherwise, update deprecated saves too (with the risk that non-English historical content may stop matching deprecations).
/* translators: %d: slide number */
ariaLabelPattern: __( 'Go to slide %d', 'rt-carousel' ),
src/blocks/carousel/deprecated.tsx:151
- PR description says the translation function was removed from
ariaLabelPatternin all save implementations (including deprecated ones), but the newSaveV201still uses__()forariaLabelPattern/announcementPattern. If the intent is not to change deprecated output (often necessary to match historically localized saved markup), the description should be updated to clarify that only current saves were de-i18n’d while deprecations retain__()for compatibility; otherwise, update deprecated saves too (with the risk that non-English historical content may stop matching deprecations).
announcementPattern: __(
'Slide {{currentSlide}} of {{totalSlides}}',
'rt-carousel',
),
examples/patterns/logo-showcase.php:22
- This pattern change introduces
\"autoplay\":true(previously omitted), which is a functional behavior change for the example, but it isn’t mentioned in the PR description. If the intent of the PR is strictly “tabs saved properly / deprecations backfill,” consider reverting this autoplay change or explicitly calling it out (and ensuring it aligns with desired default behavior for the Logo Showcase pattern).
<!-- wp:rt-carousel/carousel {"loop":true,"autoplay":true,"autoplayDelay":3000,"autoplayStopOnInteraction":false,"ariaLabel":"Partner Logos","metadata":{"categories":["rt-carousel"],"patternName":"rt-carousel/logo-showcase","name":"rtCarousel: Logo Showcase"},"className":"is-style-columns-3"} -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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