Repository navigation
feat: native flexbox layout controls for group/column/columns/grid-item - #596
Merged
ViewFromTheBox merged 5 commits intoJun 16, 2026
Merged
Conversation
…em (#595) Replace the narrow `cbInnerLayout` enum + WordPress core's minimal Flex variation with a full Flex Layout + Flex Item inspector on `artisanpack/group`, `artisanpack/column`, `artisanpack/columns`, and `artisanpack/grid-item`. Every property is per-breakpoint via the existing ViewportSwitcher. - Shared `_shared/flex-controls/` (serializer + fixtures + types + container/item React panels + parent detector + responsive helpers). - Tailwind-shaped utility class scheme (`ap-flex`, `md:ap-justify-end`, `ap-gap-x-[1rem]`, …) plus a static stylesheet and per-page arbitrary rule emission. - Blade `FlexSupport`, mirrored TS serializers in the React + Vue renderers, all asserted byte-identical against `fixtures.json` by Pest + vitest suites (15+15+14+14 = 58 parity assertions). - Group block: filter suppresses core's Flex layout variation; new deprecation migrates `layout.type === 'flex'` to `artisanpackFlex.container` on first edit. - theme.json defaults under `settings.artisanpack.flex`. - Docs: `docs/blocks/Flex-Layout.md` + crosswinds migration note + CHANGELOG entry. Closes #595 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- Fix Vue serializer header (copy-paste from React renderer left "React renderer" in the JSDoc). - Validate file_get_contents + json_decode in the Blade FlexSupport Pest fixture loader so a missing or malformed fixtures.json fails with a clear message instead of a cryptic null access. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds a native flex layout inspector panel to ChangesNative Flex Layout Panel
Sequence Diagram(s)sequenceDiagram
participant Editor as WP Block Editor
participant FlexControls as FlexContainerControls
participant Serializer as serializeFlex
participant BlockEdit as BlockEdit (group/column/etc)
participant SaveFn as Save Component
participant Renderer as React/Vue/Blade
Editor->>FlexControls: user changes flex property at breakpoint
FlexControls->>FlexControls: writeAt(attribute, breakpoint, value)
FlexControls->>BlockEdit: onChange(updatedArtisanpackFlex)
BlockEdit->>Serializer: serializeFlex(artisanpackFlex, registry)
Serializer-->>BlockEdit: { classes, arbitraryRules }
BlockEdit->>Editor: useBlockProps className includes flex classes
Editor->>SaveFn: save block
SaveFn->>Serializer: serializeFlex(artisanpackFlex, registry)
Serializer-->>SaveFn: { classes }
SaveFn-->>Editor: saved HTML with ap-flex-* classes
Editor->>Renderer: render saved HTML
Renderer->>Renderer: flexClassNames / FlexSupport::wrapperForBlock
Renderer-->>Editor: wrapper element with ap-flex-* classes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
resources/css/flex-layout.css (1)
105-247:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftResponsive utility coverage is incomplete versus serializer output.
Lines 105-247 define only partial breakpoint variants, while the serializer can emit a wider set (e.g.,
*:ap-items-self-start,*:ap-content-evenly,*:ap-grow-9,*:ap-order-9,xl:ap-flex-wrap-reverse,2xl:ap-flex-row-reverse). Missing selectors make those controls silently no-op at affected breakpoints. Please align responsive utility generation to the full serializer token matrix.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/css/flex-layout.css` around lines 105 - 247, The responsive flex utility classes are incomplete across multiple breakpoints. The serializer can generate variants like flex-row-reverse, flex-col-reverse, content-evenly, grow values up to 9, order values up to 9, and other combinations that are missing from the CSS media queries. Add the following missing utilities to each breakpoint section: in the sm (min-width: 640px) and md (min-width: 768px) sections, the coverage appears more complete, but verify all flex property variants match the serializer output; in the lg (min-width: 1024px) section, add the missing flex-row-reverse, flex-col-reverse, flex-wrap-reverse, justify-around, justify-evenly, and all align/order utilities that are present at lower breakpoints; in the xl (min-width: 1280px) section, significantly expand coverage to include flex-row-reverse, flex-col-reverse, flex-wrap-reverse, justify-around, justify-evenly, all items variants, all content variants, all self variants, grow-0 through grow-9, shrink-0 and shrink-1, basis utilities, and order-0 through order-9; in the 2xl (min-width: 1536px) section, add flex-col-reverse, flex-wrap-reverse, justify-around, justify-between, justify-evenly, items-start, items-end, items-baseline, items-stretch, all content variants, all self variants, grow/shrink variants, basis utilities, and order utilities to match the full serializer matrix. Ensure all utility classes defined at lower breakpoints are available at higher breakpoints to prevent silent no-ops when those selectors are generated.
🧹 Nitpick comments (4)
resources/js/visual-editor/blocks/_shared/flex-controls/fixtures.json (1)
3-231: ⚡ Quick winAdd a fixture for responsive flex deactivation behavior.
Please add a case for
enabled: { "base": true, "md": false }(after reset semantics are implemented) so parity tests lock the expected class output and prevent regressions on breakpoint deactivation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/visual-editor/blocks/_shared/flex-controls/fixtures.json` around lines 3 - 231, Add a new fixture object to the fixtures array that tests responsive flex deactivation behavior. The fixture should have a descriptive name like "responsive-deactivation" and set the input with container.enabled having base set to true and md set to false. Define the expected output to show what container classes are generated at the base breakpoint (which should include the "ap-flex" class) and what reset or deactivation class is generated at the md breakpoint to prevent regressions when flex is disabled at specific breakpoints.resources/css/flex-layout.css (1)
104-247: 🏗️ Heavy liftGenerate responsive blocks from shared token definitions to prevent drift.
The responsive sections are hand-maintained and have already diverged from base/serializer capabilities. Consider generating breakpoint variants from a single token source to keep CSS and serializers in lockstep.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/css/flex-layout.css` around lines 104 - 247, The responsive utility blocks (sm:, md:, lg:, xl:, 2xl: prefixed classes) within the media queries are hand-maintained and have become inconsistent—notice how xl and 2xl blocks have fewer properties than sm, md, and lg. Instead of manually coding each breakpoint variant, refactor the CSS generation process to create all responsive blocks programmatically from a single shared token source. This will ensure all breakpoints remain in lockstep with the base utilities and prevent future drift between the CSS output and any serializers that depend on these same token definitions.resources/js/visual-editor/blocks/group/__tests__/deprecated.test.tsx (1)
25-29: ⚡ Quick winStrengthen the migration assertion beyond array length.
This only verifies count, so a broken/reordered
#595entry could still pass. Add a focused check that the appended entry is eligible forlayout.type === 'flex'and that migration outputsartisanpackFlex.container.enabled.base === truewhile droppinglayout.type.Suggested test addition
it('ships the upstream five entries plus the `#595` flex-layout migration', () => { expect(Array.isArray(deprecated)).toBe(true); // 5 upstream-ported entries + 1 `#595` flex-layout migration appended. expect(deprecated).toHaveLength(6); + const flexMigration = deprecated[5] as { + isEligible: (attrs: Record<string, unknown>) => boolean; + migrate: (attrs: Record<string, unknown>) => Record<string, unknown> | undefined; + }; + expect(flexMigration.isEligible({ layout: { type: 'flex' } })).toBe(true); + const migrated = flexMigration.migrate({ + layout: { type: 'flex', orientation: 'vertical', justifyContent: 'center' }, + }) as Record<string, any>; + expect(migrated.artisanpackFlex.container.enabled.base).toBe(true); + expect(migrated.layout?.type).toBeUndefined(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/visual-editor/blocks/group/__tests__/deprecated.test.tsx` around lines 25 - 29, The test 'ships the upstream five entries plus the `#595` flex-layout migration' only verifies that the deprecated array has the correct length, which does not validate the actual content or behavior of the appended `#595` entry. Strengthen the test by adding assertions that check the appended migration entry is eligible for layout.type === 'flex', verify the migration outputs artisanpackFlex.container.enabled.base === true, and confirm the migration drops the layout.type property. This ensures the migration logic is functioning correctly, not just that the array has the expected count.packages/visual-editor-renderer-blade/tests/Feature/FlexSupportTest.php (1)
16-54: ⚡ Quick winAdd a wrapper-level accumulator regression test.
Current coverage validates
serialize()only. It won’t catch the Line 56 scope-collision path inFlexSupport::wrapperForBlock()when multiple blocks with different arbitrary values are rendered in the same request.💡 Suggested test shape
+it( 'accumulates arbitrary flex css for multiple distinct blocks', function () { + $accumulator = app( \ArtisanPackUI\VisualEditorRendererBlade\Services\ResponsiveCssAccumulator::class ); + + FlexSupport::wrapperForBlock( [ 'artisanpackFlex' => [ 'container' => [ 'gap' => [ 'row' => '8px' ] ] ] ] ); + FlexSupport::wrapperForBlock( [ 'artisanpackFlex' => [ 'container' => [ 'gap' => [ 'row' => '16px' ] ] ] ] ); + + $css = $accumulator->all(); + + expect( $css )->toContain( 'row-gap: 8px;' ) + ->and( $css )->toContain( 'row-gap: 16px;' ); +} );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/visual-editor-renderer-blade/tests/Feature/FlexSupportTest.php` around lines 16 - 54, Add a new test to validate the wrapper-level accumulator behavior in FlexSupport::wrapperForBlock() to catch scope-collision regressions. The test should create multiple blocks with different arbitrary values and render them in sequence within the same request context, then verify that each block's output is correct and that the arbitrary values from different blocks do not leak into or override each other. This ensures the accumulator logic properly isolates state between different block instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/blocks/Flex-Layout.md`:
- Around line 76-97: The documentation at lines 72-74 (sibling) contains
incomplete and partially incorrect claims about arbitrary-rule CSS emission.
Update that section to clarify that only the Blade renderer implements
arbitrary-rule CSS emission via inline <style data-ve-responsive> tags using
FlexSupport::buildArbitraryStyles() and ResponsiveCssAccumulator to flush
grouped CSS with `@media` queries by breakpoint. Explicitly state that React and
Vue renderers collect arbitraryRules in serializeFlex() but lack the
buildArbitraryStyles() function and CSS emission mechanism — these renderers
only expose flexClassNames() with classes only, meaning arbitrary values like
ap-gap-x-[3.5rem] will render as class names but their corresponding CSS rules
will not be emitted, breaking styling for those values in React and Vue. The
Theme.json defaults structure documentation at lines 76-97 (anchor) requires no
changes as it is verified correct and matches the actual implementation in
resources/theme-json/default-base.php.
In `@packages/visual-editor-renderer-blade/src/Support/FlexSupport.php`:
- Around line 53-57: The hardcoded scope key `flex-arbitrary` in the
ResponsiveCssAccumulator::push() call causes all arbitrary flex CSS to be
deduplicated under the same scope, which drops later blocks with different
arbitrary flex values. Instead of using the hardcoded string `flex-arbitrary`,
generate a unique scope key that incorporates the actual CSS content (such as by
creating a hash or checksum of the CSS). This ensures each distinct arbitrary
flex CSS gets its own unique scope while still allowing deduplication of
identical rules.
- Around line 393-395: The is_numeric() check on line 393 incorrectly accepts
string values like "1.5" which then coerce to integer 1 via (int) casting,
potentially emitting wrong canonical classes. Replace the is_numeric($value)
check with a regex pattern test using /^-?\d+$/ to match only true integer
strings (allowing optional leading minus sign) and reject float strings,
aligning with the TypeScript serializer's shared contract. This ensures numeric
value validation only accepts plain integer tokens, not float representations.
In `@resources/js/visual-editor/blocks/_shared/flex-controls/item-controls.tsx`:
- Around line 91-93: The current implementation uses CSS-only disabling with
pointerEvents: 'none' on the fieldsetStyle, which only blocks pointer events but
leaves keyboard accessibility intact, allowing users to still tab to and
interact with controls via keyboard. Replace the CSS-based approach by applying
a proper disabled attribute or prop to the actual form control elements within
the fieldset (at the main location in the item-controls file around lines 91-93
and the sibling location around lines 104-155). Instead of relying on opacity
and pointerEvents styling, pass the disabled variable as a prop to the form
controls themselves, ensuring they are semantically disabled and inaccessible
via both pointer and keyboard input.
In `@resources/js/visual-editor/blocks/_shared/flex-controls/serializer.ts`:
- Around line 189-193: The emitForEachBreakpoint callback in the serializer only
handles the true case for enabled, emitting the ap-flex class, but does not emit
any reset class when enabled is false. This causes false overrides at larger
breakpoints to be ineffective due to CSS cascading. Add an else branch in the
callback that emits a reset or non-flex utility class when value is false, and
ensure corresponding responsive CSS utilities are defined to support this reset
class for each breakpoint.
In `@resources/js/visual-editor/blocks/column/__tests__/edit.test.tsx`:
- Around line 43-48: The useState mock in the vi.mock block does not respect
lazy initializer semantics. When a function is passed as the initial value to
useState (a lazy initializer pattern), the mock should execute that function and
use its return value, rather than returning the function itself. Modify the
useState mock definition to check if the initial parameter is a function; if it
is, call it and use the result as the state value, otherwise use the initial
value directly. This ensures that when useActiveBreakpointValue() or other code
uses lazy initializers, the state is correctly initialized with the computed
value rather than the function object.
---
Outside diff comments:
In `@resources/css/flex-layout.css`:
- Around line 105-247: The responsive flex utility classes are incomplete across
multiple breakpoints. The serializer can generate variants like
flex-row-reverse, flex-col-reverse, content-evenly, grow values up to 9, order
values up to 9, and other combinations that are missing from the CSS media
queries. Add the following missing utilities to each breakpoint section: in the
sm (min-width: 640px) and md (min-width: 768px) sections, the coverage appears
more complete, but verify all flex property variants match the serializer
output; in the lg (min-width: 1024px) section, add the missing flex-row-reverse,
flex-col-reverse, flex-wrap-reverse, justify-around, justify-evenly, and all
align/order utilities that are present at lower breakpoints; in the xl
(min-width: 1280px) section, significantly expand coverage to include
flex-row-reverse, flex-col-reverse, flex-wrap-reverse, justify-around,
justify-evenly, all items variants, all content variants, all self variants,
grow-0 through grow-9, shrink-0 and shrink-1, basis utilities, and order-0
through order-9; in the 2xl (min-width: 1536px) section, add flex-col-reverse,
flex-wrap-reverse, justify-around, justify-between, justify-evenly, items-start,
items-end, items-baseline, items-stretch, all content variants, all self
variants, grow/shrink variants, basis utilities, and order utilities to match
the full serializer matrix. Ensure all utility classes defined at lower
breakpoints are available at higher breakpoints to prevent silent no-ops when
those selectors are generated.
---
Nitpick comments:
In `@packages/visual-editor-renderer-blade/tests/Feature/FlexSupportTest.php`:
- Around line 16-54: Add a new test to validate the wrapper-level accumulator
behavior in FlexSupport::wrapperForBlock() to catch scope-collision regressions.
The test should create multiple blocks with different arbitrary values and
render them in sequence within the same request context, then verify that each
block's output is correct and that the arbitrary values from different blocks do
not leak into or override each other. This ensures the accumulator logic
properly isolates state between different block instances.
In `@resources/css/flex-layout.css`:
- Around line 104-247: The responsive utility blocks (sm:, md:, lg:, xl:, 2xl:
prefixed classes) within the media queries are hand-maintained and have become
inconsistent—notice how xl and 2xl blocks have fewer properties than sm, md, and
lg. Instead of manually coding each breakpoint variant, refactor the CSS
generation process to create all responsive blocks programmatically from a
single shared token source. This will ensure all breakpoints remain in lockstep
with the base utilities and prevent future drift between the CSS output and any
serializers that depend on these same token definitions.
In `@resources/js/visual-editor/blocks/_shared/flex-controls/fixtures.json`:
- Around line 3-231: Add a new fixture object to the fixtures array that tests
responsive flex deactivation behavior. The fixture should have a descriptive
name like "responsive-deactivation" and set the input with container.enabled
having base set to true and md set to false. Define the expected output to show
what container classes are generated at the base breakpoint (which should
include the "ap-flex" class) and what reset or deactivation class is generated
at the md breakpoint to prevent regressions when flex is disabled at specific
breakpoints.
In `@resources/js/visual-editor/blocks/group/__tests__/deprecated.test.tsx`:
- Around line 25-29: The test 'ships the upstream five entries plus the `#595`
flex-layout migration' only verifies that the deprecated array has the correct
length, which does not validate the actual content or behavior of the appended
`#595` entry. Strengthen the test by adding assertions that check the appended
migration entry is eligible for layout.type === 'flex', verify the migration
outputs artisanpackFlex.container.enabled.base === true, and confirm the
migration drops the layout.type property. This ensures the migration logic is
functioning correctly, not just that the array has the expected count.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: da50b48b-599d-44dd-940b-d07a390cab77
📒 Files selected for processing (46)
CHANGELOG.mddocs/blocks/Flex-Layout.mddocs/migration/From-Crosswinds-Blocks.mdpackages/visual-editor-renderer-blade/resources/views/blocks/artisanpack/grid-item.blade.phppackages/visual-editor-renderer-blade/resources/views/blocks/core/column.blade.phppackages/visual-editor-renderer-blade/resources/views/blocks/core/columns.blade.phppackages/visual-editor-renderer-blade/resources/views/blocks/core/group.blade.phppackages/visual-editor-renderer-blade/src/Support/FlexSupport.phppackages/visual-editor-renderer-blade/src/VisualEditorRendererBladeServiceProvider.phppackages/visual-editor-renderer-blade/tests/Feature/FlexSupportTest.phppackages/visual-editor-renderer-react/src/blocks/artisanpack/grid.tsxpackages/visual-editor-renderer-react/src/blocks/core/layout.tsxpackages/visual-editor-renderer-react/src/support/flex-serializer.tspackages/visual-editor-renderer-react/tests/flex-serializer.test.tspackages/visual-editor-renderer-vue/src/blocks/artisanpack/grid.tspackages/visual-editor-renderer-vue/src/blocks/core/layout.tspackages/visual-editor-renderer-vue/src/support/flex-serializer.tspackages/visual-editor-renderer-vue/tests/flex-serializer.test.tsresources/css/flex-layout.cssresources/js/visual-editor/blocks/_shared/flex-controls/__tests__/serializer.test.tsresources/js/visual-editor/blocks/_shared/flex-controls/container-controls.tsxresources/js/visual-editor/blocks/_shared/flex-controls/fixtures.jsonresources/js/visual-editor/blocks/_shared/flex-controls/index.tsresources/js/visual-editor/blocks/_shared/flex-controls/item-controls.tsxresources/js/visual-editor/blocks/_shared/flex-controls/parent-detector.tsresources/js/visual-editor/blocks/_shared/flex-controls/responsive-helpers.tsresources/js/visual-editor/blocks/_shared/flex-controls/serializer.tsresources/js/visual-editor/blocks/_shared/flex-controls/types.tsresources/js/visual-editor/blocks/column/__tests__/edit.test.tsxresources/js/visual-editor/blocks/column/block.jsonresources/js/visual-editor/blocks/column/edit.tsxresources/js/visual-editor/blocks/column/save.tsxresources/js/visual-editor/blocks/columns/block.jsonresources/js/visual-editor/blocks/columns/edit.tsxresources/js/visual-editor/blocks/columns/save.tsxresources/js/visual-editor/blocks/grid-item/block.jsonresources/js/visual-editor/blocks/grid-item/edit.tsxresources/js/visual-editor/blocks/group/__tests__/deprecated.test.tsxresources/js/visual-editor/blocks/group/__tests__/edit.test.tsxresources/js/visual-editor/blocks/group/block.jsonresources/js/visual-editor/blocks/group/deprecated.tsxresources/js/visual-editor/blocks/group/edit.tsxresources/js/visual-editor/blocks/group/index.tsresources/js/visual-editor/blocks/group/save.tsxresources/js/visual-editor/blocks/group/suppress-core-flex.tsresources/theme-json/default-base.php
22 tasks
CodeRabbit re-review on PR #596 surfaced six findings; this pass addresses every one and ports the React/Vue side of the docs claim. - FlexSupport: hash the ResponsiveCssAccumulator scope key by CSS content so two blocks with distinct arbitrary flex values don't dedupe under a shared 'flex-arbitrary' bucket. - FlexSupport: tighten emitNumericClass to PHP regex /^-?\d+$/ so '"1.5"' stops collapsing to '1' and emitting a wrong canonical class. Now matches the TS serializer's plain-integer-token contract. - item-controls: switch to a real <fieldset disabled> so non-flex parent gating is semantic (keyboard + form submission), not just mouse-blocked. - serializer: emit 'ap-flex-none' for `enabled: false` overrides so `{ base: true, md: false }` actually un-flexes at md+. Mirrored across TS / Blade / React / Vue plus the static utility + responsive variants in flex-layout.css. - React/Vue BlockTree: walk the normalized tree, collect arbitraryRules via serializeFlex, and emit <style data-ve-flex-arbitrary> -- closes the cross-renderer parity gap the docs already promised. Both call the shared buildArbitraryStyles which mirrors Blade's grouped per-breakpoint @media output. - column/group edit tests: useState mock now honors the lazy initializer semantics that useActiveBreakpointValue relies on. Iframe canvas fix carried over from the manual-test pass: - canvas-styles.ts imports flex-layout.css via ?inline and inserts it right after LAYOUT_BASELINE_STYLES so the iframe receives the .ap-flex utilities (the blocks/*/*.css glob doesn't cover sheets outside blocks/). This is what made the editor canvas finally render the emitted classes during manual review. Tests: visual-editor vitest 250/251 files green (the only failing file is site-editor-app.test.tsx hitting an unrelated window.localStorage config issue). FlexSupport pest: 15 tests / 30 assertions green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ViewFromTheBox
marked this pull request as ready for review
June 16, 2026 19:08
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/visual-editor-renderer-blade/resources/assets/frontend/flex-layout.css`:
- Around line 120-266: The responsive utility classes at the xl and 2xl
breakpoints are missing many variants that exist in the base, sm, md, and lg
media queries. To fix this, add the missing responsive variants to both the xl
(min-width: 1280px) and 2xl (min-width: 1536px) media query blocks. Complete
these sections by including all the following utility variants with their
appropriate prefixes (xl: and \32xl: respectively): flex-row-reverse,
flex-col-reverse, flex-wrap-reverse, justify-around, justify-evenly,
items-start, items-end, items-baseline, all content-* properties (stretch,
start, end, center, between), all self-* properties (auto, start, end, center,
stretch), flex-grow-2, flex-shrink-0, flex-shrink-1, flex-basis-auto,
flex-basis-full, and order-0, order-1 variants. Match the structure and naming
pattern already established in the sm, md, and lg sections.
- Around line 24-29: The responsive flex layout selectors (sm:ap-flex,
md:ap-flex, lg:ap-flex, xl:ap-flex, and 2xl:ap-flex) are currently applying the
margin-block-start reset globally instead of being scoped to their respective
breakpoint media queries. Move each responsive variant into its corresponding
media query (sm at 640px, md at 768px, lg at 1024px, xl at 1280px, 2xl at
1536px) so that the margin reset only applies when that breakpoint is active.
Keep the base .ap-flex selector outside media queries as it represents the
default mobile-first state. This ensures that blocks using only sm:ap-flex won't
have margin reset applied below the 640px breakpoint, preserving proper mobile
spacing.
In
`@packages/visual-editor-renderer-blade/resources/views/blocks/core/group.blade.php`:
- Around line 27-36: The condition in the foreach loop that checks for
flex-enabled classes is incorrectly treating breakpoint-specific flex classes
(like md:ap-flex) the same as the base flex class ap-flex. This causes the
is-layout-flex class to be applied globally even when flex should only be
enabled at specific breakpoints. Modify the condition that sets $hasFlexEnabled
to true to only check for the base ap-flex class (the exact match), and remove
the str_ends_with check that matches breakpoint-prefixed variants like
md:ap-flex or sm:ap-flex.
In `@packages/visual-editor-renderer-react/src/support/flex-serializer.ts`:
- Around line 371-373: The escapeSelector function in both
packages/visual-editor-renderer-react/src/support/flex-serializer.ts and
packages/visual-editor-renderer-vue/src/support/flex-serializer.ts is missing
the plus character (+) in its regex character class. This causes classes like
ap-gap-x-[calc(100%+1rem)] to generate invalid CSS selectors. Update the regex
pattern in the character class within the escapeSelector function to include the
+ character alongside the existing escaped characters.
In `@resources/js/visual-editor/blocks/column/block.json`:
- Around line 91-95: The responsive-attributes allowlist in the
`artisanpackResponsive.attributes` array is missing the `"artisanpackFlex"`
attribute for flex-enabled blocks. Add `"artisanpackFlex"` to the attributes
array in the `supports.artisanpackResponsive` object for all flex-enabled blocks
(the column block shown here, as well as the columns block and grid-item block)
to ensure the responsive infrastructure treats flex values consistently with
other responsive attributes like spacing and width.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5e8f7bdd-a3db-4577-a828-961472a4cb36
📒 Files selected for processing (25)
docs/blocks/Flex-Layout.mdpackages/visual-editor-renderer-blade/resources/assets/frontend/flex-layout.csspackages/visual-editor-renderer-blade/resources/views/blocks/core/group.blade.phppackages/visual-editor-renderer-blade/resources/views/components/blocks-styles.blade.phppackages/visual-editor-renderer-blade/src/Support/FlexSupport.phppackages/visual-editor-renderer-blade/src/View/Components/BlocksStylesComponent.phppackages/visual-editor-renderer-react/src/BlockTree.tsxpackages/visual-editor-renderer-react/src/support/flex-serializer.tspackages/visual-editor-renderer-vue/src/BlockTree.tspackages/visual-editor-renderer-vue/src/support/flex-serializer.tsresources/css/flex-layout.cssresources/js/visual-editor/blocks/_shared/flex-controls/container-controls.tsxresources/js/visual-editor/blocks/_shared/flex-controls/item-controls.tsxresources/js/visual-editor/blocks/_shared/flex-controls/serializer.tsresources/js/visual-editor/blocks/column/__tests__/edit.test.tsxresources/js/visual-editor/blocks/column/block.jsonresources/js/visual-editor/blocks/column/index.tsresources/js/visual-editor/blocks/columns/block.jsonresources/js/visual-editor/blocks/columns/index.tsresources/js/visual-editor/blocks/grid-item/block.jsonresources/js/visual-editor/blocks/grid-item/index.tsresources/js/visual-editor/blocks/group/__tests__/edit.test.tsxresources/js/visual-editor/blocks/group/block.jsonresources/js/visual-editor/blocks/group/index.tsresources/js/visual-editor/editor/canvas-styles.ts
💤 Files with no reviewable changes (1)
- resources/js/visual-editor/blocks/_shared/flex-controls/container-controls.tsx
✅ Files skipped from review due to trivial changes (3)
- resources/js/visual-editor/blocks/grid-item/index.ts
- resources/js/visual-editor/blocks/group/index.ts
- docs/blocks/Flex-Layout.md
🚧 Files skipped from review as they are similar to previous changes (6)
- resources/js/visual-editor/blocks/group/block.json
- resources/js/visual-editor/blocks/column/tests/edit.test.tsx
- resources/js/visual-editor/blocks/group/tests/edit.test.tsx
- packages/visual-editor-renderer-blade/src/Support/FlexSupport.php
- resources/css/flex-layout.css
- resources/js/visual-editor/blocks/_shared/flex-controls/serializer.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@packages/visual-editor-renderer-blade/resources/assets/frontend/flex-layout.css`:
- Around line 120-266: The responsive utility classes at the xl and 2xl
breakpoints are missing many variants that exist in the base, sm, md, and lg
media queries. To fix this, add the missing responsive variants to both the xl
(min-width: 1280px) and 2xl (min-width: 1536px) media query blocks. Complete
these sections by including all the following utility variants with their
appropriate prefixes (xl: and \32xl: respectively): flex-row-reverse,
flex-col-reverse, flex-wrap-reverse, justify-around, justify-evenly,
items-start, items-end, items-baseline, all content-* properties (stretch,
start, end, center, between), all self-* properties (auto, start, end, center,
stretch), flex-grow-2, flex-shrink-0, flex-shrink-1, flex-basis-auto,
flex-basis-full, and order-0, order-1 variants. Match the structure and naming
pattern already established in the sm, md, and lg sections.
- Around line 24-29: The responsive flex layout selectors (sm:ap-flex,
md:ap-flex, lg:ap-flex, xl:ap-flex, and 2xl:ap-flex) are currently applying the
margin-block-start reset globally instead of being scoped to their respective
breakpoint media queries. Move each responsive variant into its corresponding
media query (sm at 640px, md at 768px, lg at 1024px, xl at 1280px, 2xl at
1536px) so that the margin reset only applies when that breakpoint is active.
Keep the base .ap-flex selector outside media queries as it represents the
default mobile-first state. This ensures that blocks using only sm:ap-flex won't
have margin reset applied below the 640px breakpoint, preserving proper mobile
spacing.
In
`@packages/visual-editor-renderer-blade/resources/views/blocks/core/group.blade.php`:
- Around line 27-36: The condition in the foreach loop that checks for
flex-enabled classes is incorrectly treating breakpoint-specific flex classes
(like md:ap-flex) the same as the base flex class ap-flex. This causes the
is-layout-flex class to be applied globally even when flex should only be
enabled at specific breakpoints. Modify the condition that sets $hasFlexEnabled
to true to only check for the base ap-flex class (the exact match), and remove
the str_ends_with check that matches breakpoint-prefixed variants like
md:ap-flex or sm:ap-flex.
In `@packages/visual-editor-renderer-react/src/support/flex-serializer.ts`:
- Around line 371-373: The escapeSelector function in both
packages/visual-editor-renderer-react/src/support/flex-serializer.ts and
packages/visual-editor-renderer-vue/src/support/flex-serializer.ts is missing
the plus character (+) in its regex character class. This causes classes like
ap-gap-x-[calc(100%+1rem)] to generate invalid CSS selectors. Update the regex
pattern in the character class within the escapeSelector function to include the
+ character alongside the existing escaped characters.
In `@resources/js/visual-editor/blocks/column/block.json`:
- Around line 91-95: The responsive-attributes allowlist in the
`artisanpackResponsive.attributes` array is missing the `"artisanpackFlex"`
attribute for flex-enabled blocks. Add `"artisanpackFlex"` to the attributes
array in the `supports.artisanpackResponsive` object for all flex-enabled blocks
(the column block shown here, as well as the columns block and grid-item block)
to ensure the responsive infrastructure treats flex values consistently with
other responsive attributes like spacing and width.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5e8f7bdd-a3db-4577-a828-961472a4cb36
📒 Files selected for processing (25)
docs/blocks/Flex-Layout.mdpackages/visual-editor-renderer-blade/resources/assets/frontend/flex-layout.csspackages/visual-editor-renderer-blade/resources/views/blocks/core/group.blade.phppackages/visual-editor-renderer-blade/resources/views/components/blocks-styles.blade.phppackages/visual-editor-renderer-blade/src/Support/FlexSupport.phppackages/visual-editor-renderer-blade/src/View/Components/BlocksStylesComponent.phppackages/visual-editor-renderer-react/src/BlockTree.tsxpackages/visual-editor-renderer-react/src/support/flex-serializer.tspackages/visual-editor-renderer-vue/src/BlockTree.tspackages/visual-editor-renderer-vue/src/support/flex-serializer.tsresources/css/flex-layout.cssresources/js/visual-editor/blocks/_shared/flex-controls/container-controls.tsxresources/js/visual-editor/blocks/_shared/flex-controls/item-controls.tsxresources/js/visual-editor/blocks/_shared/flex-controls/serializer.tsresources/js/visual-editor/blocks/column/__tests__/edit.test.tsxresources/js/visual-editor/blocks/column/block.jsonresources/js/visual-editor/blocks/column/index.tsresources/js/visual-editor/blocks/columns/block.jsonresources/js/visual-editor/blocks/columns/index.tsresources/js/visual-editor/blocks/grid-item/block.jsonresources/js/visual-editor/blocks/grid-item/index.tsresources/js/visual-editor/blocks/group/__tests__/edit.test.tsxresources/js/visual-editor/blocks/group/block.jsonresources/js/visual-editor/blocks/group/index.tsresources/js/visual-editor/editor/canvas-styles.ts
💤 Files with no reviewable changes (1)
- resources/js/visual-editor/blocks/_shared/flex-controls/container-controls.tsx
✅ Files skipped from review due to trivial changes (3)
- resources/js/visual-editor/blocks/grid-item/index.ts
- resources/js/visual-editor/blocks/group/index.ts
- docs/blocks/Flex-Layout.md
🚧 Files skipped from review as they are similar to previous changes (6)
- resources/js/visual-editor/blocks/group/block.json
- resources/js/visual-editor/blocks/column/tests/edit.test.tsx
- resources/js/visual-editor/blocks/group/tests/edit.test.tsx
- packages/visual-editor-renderer-blade/src/Support/FlexSupport.php
- resources/css/flex-layout.css
- resources/js/visual-editor/blocks/_shared/flex-controls/serializer.ts
🛑 Comments failed to post (5)
packages/visual-editor-renderer-blade/resources/assets/frontend/flex-layout.css (2)
24-29:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the responsive margin reset to its breakpoint media queries.
sm:/md:/lg:/xl:/2xl:selectors on Line 25–Line 29 are currently global, so a block using onlysm:ap-flexstill getsmargin-block-start: 0below640px. That breaks non-flex mobile spacing.Proposed fix
:where(.ap-flex) > * + *, -:where(.sm\:ap-flex) > * + *, -:where(.md\:ap-flex) > * + *, -:where(.lg\:ap-flex) > * + *, -:where(.xl\:ap-flex) > * + *, -:where(.\32xl\:ap-flex) > * + * { margin-block-start: 0; } +:where(.sm\:ap-flex) > * + *, +:where(.md\:ap-flex) > * + *, +:where(.lg\:ap-flex) > * + *, +:where(.xl\:ap-flex) > * + *, +:where(.\32xl\:ap-flex) > * + * { margin-block-start: 0; } + +@media (min-width: 640px) { :where(.sm\:ap-flex) > * + * { margin-block-start: 0; } } +@media (min-width: 768px) { :where(.md\:ap-flex) > * + * { margin-block-start: 0; } } +@media (min-width: 1024px) { :where(.lg\:ap-flex) > * + * { margin-block-start: 0; } } +@media (min-width: 1280px) { :where(.xl\:ap-flex) > * + * { margin-block-start: 0; } } +@media (min-width: 1536px) { :where(.\32xl\:ap-flex) > * + * { margin-block-start: 0; } }📝 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.:where(.ap-flex) > * + * { margin-block-start: 0; } `@media` (min-width: 640px) { :where(.sm\:ap-flex) > * + * { margin-block-start: 0; } } `@media` (min-width: 768px) { :where(.md\:ap-flex) > * + * { margin-block-start: 0; } } `@media` (min-width: 1024px) { :where(.lg\:ap-flex) > * + * { margin-block-start: 0; } } `@media` (min-width: 1280px) { :where(.xl\:ap-flex) > * + * { margin-block-start: 0; } } `@media` (min-width: 1536px) { :where(.\32xl\:ap-flex) > * + * { margin-block-start: 0; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/visual-editor-renderer-blade/resources/assets/frontend/flex-layout.css` around lines 24 - 29, The responsive flex layout selectors (sm:ap-flex, md:ap-flex, lg:ap-flex, xl:ap-flex, and 2xl:ap-flex) are currently applying the margin-block-start reset globally instead of being scoped to their respective breakpoint media queries. Move each responsive variant into its corresponding media query (sm at 640px, md at 768px, lg at 1024px, xl at 1280px, 2xl at 1536px) so that the margin reset only applies when that breakpoint is active. Keep the base .ap-flex selector outside media queries as it represents the default mobile-first state. This ensures that blocks using only sm:ap-flex won't have margin reset applied below the 640px breakpoint, preserving proper mobile spacing.
120-266:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftResponsive utility coverage is incomplete vs the emitted class contract.
Base utilities include tokens that are missing at responsive levels (especially
xl/2xl; e.g., Line 247–Line 257 and Line 260–Line 266 omit many direction/wrap/justify/items/content/self/grow/shrink/basis/order variants present in base). With breakpoint-aware serializer emission, those classes become no-ops at runtime.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/visual-editor-renderer-blade/resources/assets/frontend/flex-layout.css` around lines 120 - 266, The responsive utility classes at the xl and 2xl breakpoints are missing many variants that exist in the base, sm, md, and lg media queries. To fix this, add the missing responsive variants to both the xl (min-width: 1280px) and 2xl (min-width: 1536px) media query blocks. Complete these sections by including all the following utility variants with their appropriate prefixes (xl: and \32xl: respectively): flex-row-reverse, flex-col-reverse, flex-wrap-reverse, justify-around, justify-evenly, items-start, items-end, items-baseline, all content-* properties (stretch, start, end, center, between), all self-* properties (auto, start, end, center, stretch), flex-grow-2, flex-shrink-0, flex-shrink-1, flex-basis-auto, flex-basis-full, and order-0, order-1 variants. Match the structure and naming pattern already established in the sm, md, and lg sections.packages/visual-editor-renderer-blade/resources/views/blocks/core/group.blade.php (1)
27-36:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly force
is-layout-flexwhen base flex is enabled.Lines 29-35 currently treat breakpoint-only classes (
md:ap-flex, etc.) as globally flex-enabled. That forces basedisplay:flexvia.is-layout-flex, so mobile can render flex even when flex was intended only for a larger breakpoint.🔧 Suggested patch
- $hasFlexEnabled = false; - foreach ( $flexClasses as $cls ) { - if ( 'ap-flex' === $cls || str_ends_with( $cls, ':ap-flex' ) ) { - $hasFlexEnabled = true; - break; - } - } - if ( '' === $layoutType && $hasFlexEnabled ) { + $hasBaseFlexEnabled = in_array( 'ap-flex', $flexClasses, true ); + if ( '' === $layoutType && $hasBaseFlexEnabled ) { $layoutClass = 'is-layout-flex'; }📝 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.$hasBaseFlexEnabled = in_array( 'ap-flex', $flexClasses, true ); if ( '' === $layoutType && $hasBaseFlexEnabled ) { $layoutClass = 'is-layout-flex'; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/visual-editor-renderer-blade/resources/views/blocks/core/group.blade.php` around lines 27 - 36, The condition in the foreach loop that checks for flex-enabled classes is incorrectly treating breakpoint-specific flex classes (like md:ap-flex) the same as the base flex class ap-flex. This causes the is-layout-flex class to be applied globally even when flex should only be enabled at specific breakpoints. Modify the condition that sets $hasFlexEnabled to true to only check for the base ap-flex class (the exact match), and remove the str_ends_with check that matches breakpoint-prefixed variants like md:ap-flex or sm:ap-flex.packages/visual-editor-renderer-react/src/support/flex-serializer.ts (1)
371-373:
⚠️ Potential issue | 🟡 Minor🧩 Analysis chain
🏁 Script executed:
#!/bin/bash python - <<'PY' import re sample = "md:ap-gap-x-[calc(100%+1rem)]" pattern = re.compile(r'[\[\]():.,#%/\s]') escaped = pattern.sub(lambda m: '\\' + m.group(0), sample) print("sample :", sample) print("escaped :", escaped) print("plus escaped?", "\\+" in escaped) PY rg -n "escapeSelector|replace\\( /\\[\\\\\\[\\\\\\]\\(\\):\\.,#%/\\\\s\\]/g" --type=tsRepository: ArtisanPack-UI/visual-editor
Length of output: 1199
🏁 Script executed:
# Check the exact content of both files at the mentioned lines echo "=== React file (lines 369-373) ===" sed -n '369,373p' packages/visual-editor-renderer-react/src/support/flex-serializer.ts echo -e "\n=== Vue file (lines 359-363) ===" sed -n '359,363p' packages/visual-editor-renderer-vue/src/support/flex-serializer.ts # Look for tests or usages of escapeSelector to understand the impact echo -e "\n=== Usages of escapeSelector ===" rg -n "escapeSelector\(" --type=ts -A 2 -B 2Repository: ArtisanPack-UI/visual-editor
Length of output: 2594
🏁 Script executed:
# Verify if '+' in CSS selectors needs escaping python3 - <<'PY' import re # Test various selectors with unescaped vs escaped '+' test_cases = [ "ap-gap-x-[calc(100%+1rem)]", "w-[100px+20px]", "text-[color:rgb(100+50)]" ] regex_current = re.compile(r'[\[\]():.,#%/\s]') regex_fixed = re.compile(r'[\[\]():.,#%/+\s]') print("Character escaping comparison:\n") for test in test_cases: escaped_current = regex_current.sub(lambda m: '\\' + m.group(0), test) escaped_fixed = regex_fixed.sub(lambda m: '\\' + m.group(0), test) print(f"Original: .{test}") print(f"Current: .{escaped_current}") print(f"Fixed: .{escaped_fixed}") print(f"Plus escaped? {'+' not in escaped_fixed}") print() PYRepository: ArtisanPack-UI/visual-editor
Length of output: 503
Add
+to the character class inescapeSelectorfor both React and Vue renderers.The
+character is not currently escaped in the regex pattern. Classes likeap-gap-x-[calc(100%+1rem)]generate invalid CSS selectors without escaping the+, causing those arbitrary rules to fail. Update bothpackages/visual-editor-renderer-react/src/support/flex-serializer.ts(line 372) andpackages/visual-editor-renderer-vue/src/support/flex-serializer.ts(line 362) to include+in the character class:Suggested patch
- return className.replace( /[\[\]():.,#%/\s]/g, ( char ) => `\\${ char }` ) + return className.replace( /[\[\]():.,#%/+\s]/g, ( char ) => `\\${ char }` )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/visual-editor-renderer-react/src/support/flex-serializer.ts` around lines 371 - 373, The escapeSelector function in both packages/visual-editor-renderer-react/src/support/flex-serializer.ts and packages/visual-editor-renderer-vue/src/support/flex-serializer.ts is missing the plus character (+) in its regex character class. This causes classes like ap-gap-x-[calc(100%+1rem)] to generate invalid CSS selectors. Update the regex pattern in the character class within the escapeSelector function to include the + character alongside the existing escaped characters.resources/js/visual-editor/blocks/column/block.json (1)
91-95:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winRegister
artisanpackFlexin the responsive-attributes allowlist for all flex-enabled blocks.
artisanpackFlexis breakpoint-aware, but this list omits it here (and inresources/js/visual-editor/blocks/columns/block.jsonandresources/js/visual-editor/blocks/grid-item/block.json). Add"artisanpackFlex"to each block’ssupports.artisanpackResponsive.attributesarray so responsive infrastructure treats flex values consistently with other responsive attributes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/js/visual-editor/blocks/column/block.json` around lines 91 - 95, The responsive-attributes allowlist in the `artisanpackResponsive.attributes` array is missing the `"artisanpackFlex"` attribute for flex-enabled blocks. Add `"artisanpackFlex"` to the attributes array in the `supports.artisanpackResponsive` object for all flex-enabled blocks (the column block shown here, as well as the columns block and grid-item block) to ensure the responsive infrastructure treats flex values consistently with other responsive attributes like spacing and width.
Second re-review on PR #596 surfaced five more findings (the inline comments failed to post due to a GitHub rate limit, so they only landed in the review summary). Addressed four; deferred one with rationale. - flex-layout.css: scope each responsive margin-block-start reset inside its own @media query. Previously `:where(.sm\:ap-flex) > * + *` applied unconditionally, so a block with only `sm:ap-flex` set would have its children's `margin-block-start` zeroed even below 640px (where the block is still block-flow). - flex-layout.css: fill out xl + 2xl media queries with the full variant set sm/md/lg already define (flex-row-reverse, flex-wrap, every justify/items/content/self, grow/shrink/basis/order). The TS serializer emits `xl:ap-flex-row-reverse` etc. when the author sets those values; without matching CSS the classes were silently dropped. - group.blade.php: only flip the wrapper to `is-layout-flex` when the base `ap-flex` class is present, not when a breakpoint-prefixed variant like `md:ap-flex` is present. Prefixed variants mean "flex at md+", so flipping unconditionally would apply flex below the breakpoint too. The matching `:where(.md\:ap-flex) > * + *` reset now lives inside its own media query (see above), so we no longer need the wrapper-flip safety net. - escapeSelector (React + Vue + Blade): add `+` to the escape regex so arbitrary values like `ap-gap-x-[calc(100%+1rem)]` produce a valid CSS selector instead of breaking on the unescaped `+`. Deferred (with comment in the PR): - Adding `"artisanpackFlex"` back to `supports.artisanpackResponsive.attributes` on column / columns / grid-item / group block.json. Was intentionally removed during manual testing: the responsive HOC mis-routes nested-object attributes (artisanpackFlex is a {container, item} shape), causing `block-validation` to mark posts as "Block contains unexpected or invalid content" on every load. The serializer manages its own per-breakpoint cascade internally via the BreakpointRegistry, so the attribute does not need to participate in the generic responsive-spread infrastructure. Tests: visual-editor vitest 250/251 files green (same 17 pre-existing site-editor failures from a window.localStorage config issue unrelated to this PR). FlexSupport pest: 15 tests / 30 assertions green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Member
Author
Round-three CodeRabbit pass: deferred findingThe latest review (commit b0ef484) addressed four of the five new findings — escape regex + Deferred: adding This was intentionally removed during manual testing earlier in the PR. The responsive HOC mis-routes nested-object attributes — The flex serializer manages its own per-breakpoint cascade internally (via Happy to revisit if a follow-up changes the HOC to handle nested-object attributes; this thread is the breadcrumb. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/visual-editor-renderer-vue/src/support/flex-serializer.ts`:
- Around line 361-365: The escapeSelector function uses an incomplete whitelist
regex that only escapes specific characters, missing others like asterisk that
appear in arbitrary class values. Replace the whitelist approach with a
blacklist that escapes all characters except those explicitly safe in CSS
identifiers: alphanumeric characters (a-z, A-Z, 0-9), hyphens, and underscores.
Update the regex pattern in escapeSelector to match anything that is NOT in this
safe set, ensuring all CSS-significant characters in arbitrary values like calc
expressions are properly escaped to prevent malformed selectors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9e1790f0-d362-456c-92e0-4947799f829d
📒 Files selected for processing (6)
packages/visual-editor-renderer-blade/resources/assets/frontend/flex-layout.csspackages/visual-editor-renderer-blade/resources/views/blocks/core/group.blade.phppackages/visual-editor-renderer-blade/src/Support/FlexSupport.phppackages/visual-editor-renderer-react/src/support/flex-serializer.tspackages/visual-editor-renderer-vue/src/support/flex-serializer.tsresources/css/flex-layout.css
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/visual-editor-renderer-blade/resources/views/blocks/core/group.blade.php
- resources/css/flex-layout.css
- packages/visual-editor-renderer-react/src/support/flex-serializer.ts
- packages/visual-editor-renderer-blade/src/Support/FlexSupport.php
Round-four CodeRabbit follow-up. The allowlist-style escapeSelector keeps growing as new arbitrary values surface (`+` last round; CodeRabbit's next example was `*`). Switched all three ports — React, Vue, Blade — to a denylist that escapes everything outside the CSS-identifier safe set `[A-Za-z0-9_-]`. Now arbitrary values that ship with `*`, `~`, `>`, `=`, `&`, `^`, `$`, `|`, etc. all serialize to a valid selector without revisiting the regex. Output is a superset of what the previous allowlists escaped, so the shared `fixtures.json` parity tests continue to pass — they assert on the structured `arbitraryRules` shape, not on `buildArbitraryStyles` output, so the wider escape set doesn't shift the fixtures. Tests: React + Vue vitest 29/29 files / 429 tests green. FlexSupport pest 15 tests / 30 assertions green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
github-project-automation
Bot
moved this from In Review
to Done
in ArtisanPack UI Overview
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.
Description
Native Flex Layout + Flex Item inspector panels on
artisanpack/group,artisanpack/column,artisanpack/columns, andartisanpack/grid-item. Every flexbox property is exposed per-breakpoint via the existing<ViewportSwitcher />, replacing both core's narrow Flex layout variation (on group) and crosswinds-blocks'cbInnerLayoutenum.Closes: #595
Type of Change
Related Issue
Issue: #595
Motivation and Context
WordPress core's Flex layout variation exposes only direction, justify, vertical alignment, wrap, and blockGap — and only when
layout.type === 'flex'is set. Crosswinds-blocks' legacy plugin solved a real authoring problem (fill-height, push-to-bottom, center-children) but masked the full flexbox surface behind a five-option enum. Since we own all four affected blocks, we can ship a first-class Flex panel with the full property surface and per-breakpoint responsiveness.Changes Made
resources/js/visual-editor/blocks/_shared/flex-controls/. Single source of truth for class generation;fixtures.jsondrives parity tests across all 4 packages.FlexContainerControls(enable, direction, wrap, justify, align-items, align-content, place-content, gap) +FlexItemControls(align-self, grow, shrink, basis, order) +useFlexParentwalking the block tree once for the disabled state.artisanpackFlexattribute added to group / column / columns / grid-item, plus entry in each block'ssupports.artisanpackResponsive.attributes.layout.type === 'flex'→artisanpackFlex.containeron first edit. Unedited content unchanged.artisanpack/groupvia ablocks.registerBlockTypefilter.resources/css/flex-layout.css(ap-flex-*classes withsm:/md:/lg:/xl:/2xl:prefixes). Arbitrary values likeap-gap-x-[16px]emit per-page via the renderer.packages/visual-editor-renderer-blade):FlexSupport.phpmirrors the TS serializer; arbitrary-value rules push into the existingResponsiveCssAccumulator. Templates for group / column / columns / grid-item all merge the new classes.support/flex-serializer.tsports the JS serializer with the same fixtures. Block components emit the same classes.settings.artisanpack.flex(enable / defaultDirection / defaultJustifyContent / defaultAlignItems / defaultGap).docs/blocks/Flex-Layout.md,docs/migration/From-Crosswinds-Blocks.md, CHANGELOG entry under[Unreleased].How Has This Been Tested?
Testing Environment:
Tests Performed:
fixtures.jsonruns through 4 test suites (editor, Blade, React, Vue). 58 parity assertions total; all green.artisanpack-ui-dev/tests/Feature/VisualEditor/FlexRendererTest.phpexercise the full renderer registration withartisanpackFlexattributes; all assertions pass.Accessibility Tests Run
<ToggleGroupControl>primitives inherit core's keyboard/focus management.labelprops on every WP component.Details: Icon-button groups are stock
@wordpress/componentsToggleGroupControl, identical to core'sjustifyContentUI. The Advanced section uses native<details>/<summary>. Reverse direction variants surface a written hint about visual vs DOM order.Tests Added
Test details:
resources/js/visual-editor/blocks/_shared/flex-controls/__tests__/serializer.test.ts— 15 fixtures + null-input case (editor TS serializer).packages/visual-editor-renderer-blade/tests/Feature/FlexSupportTest.php— 14 fixtures + null-input case (Blade PHP mirror).packages/visual-editor-renderer-react/tests/flex-serializer.test.ts— 14 fixtures (React TS mirror).packages/visual-editor-renderer-vue/tests/flex-serializer.test.ts— 14 fixtures (Vue TS mirror).@wordpress/componentsand@wordpress/elementimports.Documentation
Documentation details:
docs/blocks/Flex-Layout.md— full inspector reference: panel locations, every container/item control with CSS property + values, generated class shape, theme.json defaults, renderer parity story.docs/migration/From-Crosswinds-Blocks.md— hand-migration table mappingcbUseFlex/cbFillHeight/cbInnerLayoutto the new attribute shape.CHANGELOG.md— entry under[Unreleased].Screenshots
Not included in this draft — visual verification requires the editor canvas + the dev-app's
/components/flex-layoutroute (also added in this PR via the artisanpack-ui-dev repo).Pre-Submission Checklist
Notes for Reviewer
[base, sm, md, lg, xl, 2xl]. This order is asserted by the fixture set and must hold across all 4 implementations./components/flex-layoutroute + Pest test) — those land separately.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes