From 99359fa0fa817fd8e814a1f1d7223ba4eda1c187 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Wed, 24 Jun 2026 12:49:33 +0300 Subject: [PATCH] fix(editor): load the float responsive rule + test applyAlignment (#145 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #157 (Request changes) caught two blockers: 1. DEAD responsive CSS: the `@media (max-width:600px)` float-reset was added to `image-resize.module.css`, which is imported NOWHERE — the image container's classes come from `common/node-resize.module.css` (via buildResizeClasses). So on mobile a floated image kept its px width + float and crushed the text, exactly the failure the rule promised to prevent. Moved the rule to `common/node-resize.module.css` (the module actually imported by the resize node views); its `:global([data-image-align=...])` selectors are data-attr based, so they work unchanged. Reverted the dead addition from the (pre-existing, orphaned) image-resize.module.css. 2. `applyAlignment` was untested. Exported it and added `image.spec.ts` (vitest/ jsdom) covering all five align values, the data-image-align mirror, and the floatLeft -> left reset-then-apply (the guard against a leaked float). Switched the float writes to the canonical CSSOM `cssFloat` property (portable: browsers + jsdom; behavior identical to the `.float` alias). editor-ext build + client tsc clean; 6 image.spec tests green. Co-Authored-By: Claude Opus 4.8 --- .../components/common/node-resize.module.css | 15 +++++ .../components/image/image-resize.module.css | 14 ---- .../editor-ext/src/lib/image/image.spec.ts | 67 +++++++++++++++++++ packages/editor-ext/src/lib/image/image.ts | 8 +-- 4 files changed, 86 insertions(+), 18 deletions(-) create mode 100644 packages/editor-ext/src/lib/image/image.spec.ts diff --git a/apps/client/src/features/editor/components/common/node-resize.module.css b/apps/client/src/features/editor/components/common/node-resize.module.css index 4159e44e..ec72f955 100644 --- a/apps/client/src/features/editor/components/common/node-resize.module.css +++ b/apps/client/src/features/editor/components/common/node-resize.module.css @@ -73,3 +73,18 @@ display: none !important; } } + +/* Float image (#145): on narrow screens a floated image would crowd the text to + an unreadable column, so collapse it to full width and drop the float. + `!important` is required because applyAlignment sets `float`/`padding` inline, + which a normal rule cannot override. Keys off the `data-image-align` attribute + the image node view mirrors onto its container. This module is the one actually + imported by the resize node views (node-resize-handles.ts), so the rule loads. */ +@media (max-width: 600px) { + :global([data-image-align="floatLeft"]), + :global([data-image-align="floatRight"]) { + float: none !important; + width: 100% !important; + padding: 0 !important; + } +} diff --git a/apps/client/src/features/editor/components/image/image-resize.module.css b/apps/client/src/features/editor/components/image/image-resize.module.css index e527fae0..24414171 100644 --- a/apps/client/src/features/editor/components/image/image-resize.module.css +++ b/apps/client/src/features/editor/components/image/image-resize.module.css @@ -62,17 +62,3 @@ .resizing .handleBar { background-color: light-dark(var(--mantine-color-blue-6), var(--mantine-color-blue-4)); } - -/* Float image (#145): on narrow screens a floated image would crowd the text to - an unreadable column, so collapse it to full width and drop the float. - `!important` is required because applyAlignment sets `float`/`padding` inline, - which a normal rule cannot override. Keys off the data-image-align attribute - the node view mirrors onto the container. */ -@media (max-width: 600px) { - :global([data-image-align="floatLeft"]), - :global([data-image-align="floatRight"]) { - float: none !important; - width: 100% !important; - padding: 0 !important; - } -} diff --git a/packages/editor-ext/src/lib/image/image.spec.ts b/packages/editor-ext/src/lib/image/image.spec.ts new file mode 100644 index 00000000..2a1b7f8c --- /dev/null +++ b/packages/editor-ext/src/lib/image/image.spec.ts @@ -0,0 +1,67 @@ +import { describe, it, expect, beforeEach } from "vitest"; +import { applyAlignment } from "./image"; + +// applyAlignment is a pure DOM mutation: it sets the float / padding / +// justify-content / data-image-align on an image node-view container per the +// resolved `align`. Tested directly (issue #145 review) since the five-way +// branch, the reset-then-apply guard, and the data-image-align mirror (which the +// responsive @media rule keys off) are otherwise uncovered. + +describe("applyAlignment", () => { + let el: HTMLElement; + beforeEach(() => { + el = document.createElement("div"); + }); + + it("floatLeft -> float:left + right padding, mirrored on data-image-align", () => { + applyAlignment(el, "floatLeft"); + expect(el.style.cssFloat).toBe("left"); + expect(el.style.padding).toBe("0px 10px 0px 0px"); + expect(el.dataset.imageAlign).toBe("floatLeft"); + expect(el.style.justifyContent).toBe("flex-start"); + }); + + it("floatRight -> float:right + left padding", () => { + applyAlignment(el, "floatRight"); + expect(el.style.cssFloat).toBe("right"); + expect(el.style.padding).toBe("0px 0px 0px 10px"); + expect(el.dataset.imageAlign).toBe("floatRight"); + expect(el.style.justifyContent).toBe("flex-end"); + }); + + it("left -> justify flex-start, no float", () => { + applyAlignment(el, "left"); + expect(el.style.justifyContent).toBe("flex-start"); + expect(el.style.cssFloat).toBe(""); + expect(el.style.padding).toBe(""); + expect(el.dataset.imageAlign).toBe("left"); + }); + + it("right -> justify flex-end, no float", () => { + applyAlignment(el, "right"); + expect(el.style.justifyContent).toBe("flex-end"); + expect(el.style.cssFloat).toBe(""); + expect(el.dataset.imageAlign).toBe("right"); + }); + + it("center (default) -> justify center, no float", () => { + applyAlignment(el, "center"); + expect(el.style.justifyContent).toBe("center"); + expect(el.style.cssFloat).toBe(""); + expect(el.style.padding).toBe(""); + expect(el.dataset.imageAlign).toBe("center"); + }); + + it("clears a previous float when switching floatLeft -> left (reset-then-apply)", () => { + applyAlignment(el, "floatLeft"); + expect(el.style.cssFloat).toBe("left"); + expect(el.style.padding).toBe("0px 10px 0px 0px"); + // Switching to a block alignment must drop the float and its padding, not + // leak them (the bug the reset guard prevents). + applyAlignment(el, "left"); + expect(el.style.cssFloat).toBe(""); + expect(el.style.padding).toBe(""); + expect(el.dataset.imageAlign).toBe("left"); + expect(el.style.justifyContent).toBe("flex-start"); + }); +}); diff --git a/packages/editor-ext/src/lib/image/image.ts b/packages/editor-ext/src/lib/image/image.ts index 50e40491..7856ecb6 100644 --- a/packages/editor-ext/src/lib/image/image.ts +++ b/packages/editor-ext/src/lib/image/image.ts @@ -376,10 +376,10 @@ export const TiptapImage = Image.extend({ }, }); -function applyAlignment(container: HTMLElement, align: string) { +export function applyAlignment(container: HTMLElement, align: string) { // Reset the float-mode styles first so toggling between any two modes is clean // (a previous float must not leak into a later left/center/right). - container.style.float = ""; + container.style.cssFloat = ""; container.style.padding = ""; // Mirror the resolved alignment onto the CONTAINER as a data attribute so the // responsive stylesheet can neutralize the float on small screens (an inline @@ -389,11 +389,11 @@ function applyAlignment(container: HTMLElement, align: string) { if (align === "floatLeft") { // Real text wrap: the (shrink-to-fit) container floats left, text flows on // its right. The inner already carries max-width:100%. - container.style.float = "left"; + container.style.cssFloat = "left"; container.style.padding = "0 10px 0 0"; container.style.justifyContent = "flex-start"; } else if (align === "floatRight") { - container.style.float = "right"; + container.style.cssFloat = "right"; container.style.padding = "0 0 0 10px"; container.style.justifyContent = "flex-end"; } else if (align === "left") {