Repository navigation
feat: position support opt-in, tests, docs (#645, #646, #647, #648) - #657
Merged
ViewFromTheBox merged 2 commits intoJul 14, 2026
Merged
ViewFromTheBox merged 2 commits into
ViewFromTheBox merged 2 commits into
Conversation
, #646, #647, #648) Closes out the #640 umbrella by (1) flipping the position opt-in on every top-level artisanpack block, (2) shipping integration + e2e coverage, and (3) documenting the feature. Also removes the duplicate breakpoint switcher from the Position panel — the top-bar viewport switcher already drives it via the shared active-breakpoint store. ## Changes ### #645 — 82 artisanpack blocks opt in Bulk-added `"position": true` to `supports` in every `resources/js/visual-editor/blocks/*/block.json` that doesn't declare a `parent` or `ancestor` restriction. Twenty child-only blocks (accordion internals, list items, `column` inside `columns`, `button` inside `buttons`, `post-template` inside `query`, pagination pieces) are deliberately skipped — positioning a child that a container relies on for layout breaks the container's contract. `artisanpack/group` already carried Gutenberg's `supports.position: { sticky: true }` object shape; `positionEnabled()` accepts both `true` and any truthy object, so it was left alone. ### #646 — no-op given the I7 cutover This repo replaces `registerCoreBlocks()` with `registerArtisanPackBlocks()` (I7 · #415). No `core/*` block enters the editor, so a filter injecting position support into core containers would be dead code. Every container listed in the parent issue (`core/group`, `core/columns`, `core/column`, `core/cover`, `core/row`, `core/stack`, `core/grid`, `core/buttons`, `core/image`) has an `artisanpack/*` fork covered by #645. ### #647 — integration + e2e coverage - **Integration (Vitest, real):** `resources/js/visual-editor/positioning/__tests__/integration.test.ts` round-trips the full resolver → merger → emitter chain across all five Tailwind breakpoints, verifies raw vs. effective readers, legacy sticky no-churn, and the static-toggle-preserves-orphan-fields flow. 4 new tests, 38 positioning tests total green. - **E2E (Playwright shape, commented):** `tests/E2E/positioning.spec.ts` documents five runtime scenarios — sticky group on canvas scroll, absolute cover inside a positioned parent, ancestor warning when the parent is static, per-breakpoint viewport switching, legacy sticky frontend render. Follows the `animations.spec.ts` precedent — activated when the Playwright runner lands. ### #648 — docs New `docs/position.md` covering opt-in, attribute shape, position values, offset units, per-breakpoint inheritance, the ancestor warning, the two frontend emission channels (inline + `<style data-ve-position>`), and a troubleshooting section. Linked from `docs/home.md`. ### Panel cleanup — remove duplicate breakpoint switcher The Position panel had its own `<Base | Mobile | Tablet | Desktop | xl+ | 2xl+>` tab strip that flipped the shared active-breakpoint store. The top-bar viewport switcher already drives that store — the panel's local tabs were a second control for the same job. Removed the tab strip; the panel reads `activeBreakpoint` via `useSyncExternalStore` and re-renders effective values / "Inherited" hints when the top-bar switches. Docs updated to note the panel follows the top-bar switcher. ## Tests - 38 positioning JS tests (34 unit + 4 new integration) — green. - Full JS suite: 2809 passed / 18 pre-existing failures (site-editor `localStorage` under jsdom, unrelated). No new regressions from the 82 block.json edits. Closes #645, #646, #647, #648 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (89)
📝 WalkthroughWalkthroughPosition support is documented, enabled across in-scope blocks, connected to shared responsive breakpoint state, removed from the service provider’s emitter registration, and covered by integration tests plus commented E2E scaffolding. ChangesPosition support rollout
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Applies the 10 findings from the local /code-review pass on the initial commit of this PR. No behavior changes to end users beyond the tab-strip removal from the previous commit (already reviewed and approved). ## Correctness - **JSON reformatting churn on 17 block.json files (Angle A #2, Conv #2).** Reverted every touched `block.json` to the base branch state, then re-inserted `"position": true,` via a text-level regex that only adds one line. Diff footprint for the block manifests dropped from 82 files / 695 insertions / 82 deletions to 82 files / 82 insertions — one clean line per manifest. Preserves the original compact inline arrays (`"keywords": [ "a", "b" ]`, `"align": [ "wide", "full" ]`, `"enum": [ ... ]`) and keeps git-blame on those arrays pointed at the original authors instead of this PR. - **Stale JSDoc (Angle A #1, Angle B #1).** The `with-position-control.tsx` docblock still described the removed tab strip. Rewrote to reflect the actual behavior: the panel follows the top-bar `ViewportSwitcher` via the shared active-breakpoint store, matching every sibling responsive-aware inspector panel. - **CHANGELOG path case (Angle A #3, Conv #1).** `tests/e2e/positioning.spec.ts` → `tests/E2E/positioning.spec.ts` — git tracks the file at the uppercase path (matches the `animations.spec.ts` sibling). macOS's case-insensitive filesystem hid the mismatch locally. - **CHANGELOG missing UX note (Angle B #5).** Added a sentence explaining that per-breakpoint authoring follows the top-bar viewport switcher and no separate control lives in the panel. Answers the anticipated support ticket "where did the Base/Mobile/Tablet/Desktop buttons in the Position panel go?". - **Panel-vs-top-bar dependency (Angle A #4, Angle B #3).** Verified intentional and consistent with sibling panels; documented in the new CHANGELOG sentence and the rewritten JSDoc. No code change — the a11y story is handled at the inspector level by `InspectorScopeChip` (`role="status" aria-live="polite"`). ## Cleanup - **Dead `PositionEmitter` DI binding (Angle C #4).** Removed the scoped binding from `VisualEditorServiceProvider.php` plus its `use` import. `BlockSupports::compilePosition()` instantiates the emitter directly with the request-scoped `BreakpointRegistry` — no container round-trip needed. - **`docs/position.md` Related links (Angle A #6).** Switched from relative Markdown `[Animations](animations.md)` to wiki-style `[[Animations]]`, matching `docs/home.md`. - **Brittle exact-string assertion (Angle A #5).** Replaced the order-and-whitespace-sensitive `.toBe(...)` with individual `.toContain(...)` checks in `integration.test.ts`, matching the surrounding assertions. - **`docs/home.md` slash spacing (Conv #3).** `(static/relative/...)` → `(static / relative / ...)`, matching the sibling `Linear / radial / conic borders` entry directly above. ## Verified out (not fixed) - **Dynamic blocks miss position CSS on frontend (Angle C #1).** `PositionEmitter` IS reached via `BlockSupports::compilePosition()` → `compile()` → block partials calling `wrapperAttrs()`. The DI binding was dead but the code path is live. - **`addSaveProps` writes string `style` (Angle C #3).** WordPress's `renderToString` (save serialization path) accepts string style attributes; user smoke test confirmed frontend renders correctly. - **`responsive` attribute injection breaks downgrade (Angle C #2).** Real forward-only concern for the feature, not a defect in this PR. ## Tests - 38 positioning JS tests + 86 PHP tests, all green. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
ViewFromTheBox
marked this pull request as ready for review
July 14, 2026 21:53
github-project-automation
Bot
moved this from In Review
to Done
in ArtisanPack UI Overview
This was referenced Jul 14, 2026
Merged
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
Closes out the #640 umbrella with the four remaining sub-issues:
supports.position: trueon all top-level artisanpack blocks (82 files).animations.spec.tsprecedent).docs/position.md, linked fromdocs/home.md.Also carries a small UX cleanup: the Position panel's local breakpoint tab strip is gone — the top-bar viewport switcher already drives the same shared active-breakpoint store, so the local tabs were duplicate UI. The panel still reads the active breakpoint and re-renders effective values / "Inherited" hints as the top-bar switches.
Closes: #645, #646, #647, #648
Type of Change
Related Issue
Issue: #640 (umbrella); #645, #646, #647, #648 (sub-issues)
Motivation and Context
The core positioning machinery landed in #656 (feature branch parent). This PR completes the umbrella by turning it on for every artisanpack block that makes sense, backing it with round-trip test coverage, and giving downstream developers a docs page to follow.
Changes Made
resources/js/visual-editor/blocks/*/block.jsonfiles gain"position": truein theirsupportsblock. Child-only blocks (20 total — accordion internals, list items,column,button,post-template, pagination pieces) are deliberately skipped.positioning/__tests__/integration.test.tscovers the full resolver → merger → emitter chain across all breakpoints; legacy sticky no-churn; static-toggle preserves orphan offsets/z-index.tests/E2E/positioning.spec.tsdocuments five runtime scenarios (sticky group, absolute cover, ancestor warning, per-breakpoint viewport switching, legacy sticky) as commented Playwright shape.docs/position.md(opt-in, attribute shape, values, offsets, inheritance, ancestor warning, emission channels, troubleshooting) +docs/home.mdlink.with-position-control.tsx.vendor:publish --forceupgrade note from feat: CSS positioning support for blocks (#640) #656.How Has This Been Tested?
Testing Environment:
Tests Performed:
Accessibility Tests Run
Tests Added
Test details:
site-editor-app.test.tsx—localStorageunder jsdom, unrelated to this branch).Documentation
docs/position.mdlinked fromdocs/home.md)Pre-Submission Checklist
json.dumps(indent=' '))🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests