vvzvlad/gitmost
1
0
Fork 0
Code Issues 14 Pull Requests 12 Packages Projects Releases Activity

feat(editor): float image with text wrap (#145) #157

Merged
vvzvlad merged 3 commits from feat/float-image into develop 2026-06-24 14:04:04 +03:00
Conversation 2 Commits 3 Files Changed 6 +156 -5
Ghost commented 2026-06-24 06:36:05 +03:00
Copy Link

Закрывает #145.

Что

Обтекание картинки текстом: floatLeft/floatRight — текст обтекает изображение сбоку, в дополнение к блочным left/center/right. Порт из Forkmost PR #7 / апстрим Docmost PR #1132 (fuscodev), адаптирован под нашу императивную ноду-вью картинки (у апстрима — React styled-компонент, у нас стилизуется контейнер node-view через applyAlignment).

Изменения

  • editor-ext/image.ts: setImageAlign принимает floatLeft/floatRight; applyAlignment сначала сбрасывает float/padding, затем для float-режима ставит float:left|right + боковой padding на shrink-to-fit контейнер (внутренний <img> уже с max-width:100%). Резолвнутый align зеркалится в data-image-align на контейнере для адаптивного правила. data-align уже round-trip'ится через parse/renderHTML → float переживает сериализацию/коллаборацию/историю без смены схемы.
  • image-menu.tsx: кнопки Float-left / Float-right в bubble-меню (IconFloatLeft/Right) с active-состоянием.
  • image-resize.module.css: на узких экранах (≤600px) плавающая картинка схлопывается в 100% ширины и сбрасывает float (!important, по data-image-align) — это follow-up-коммит «100% width on small screen» из Forkmost.
  • i18n: en-US + ru-RU.

Проверка

pnpm --filter @docmost/editor-ext build + client tsc --noEmit — чисто. Логика и сериализация подтверждены типами/сборкой; визуальное обтекание лучше глянуть в браузере.

🤖 Generated with Claude Code

Закрывает #145. ## Что Обтекание картинки текстом: `floatLeft`/`floatRight` — текст обтекает изображение сбоку, в дополнение к блочным left/center/right. Порт из Forkmost PR #7 / апстрим Docmost PR #1132 (fuscodev), адаптирован под нашу **императивную** ноду-вью картинки (у апстрима — React styled-компонент, у нас стилизуется контейнер node-view через `applyAlignment`). ## Изменения - `editor-ext/image.ts`: `setImageAlign` принимает `floatLeft`/`floatRight`; `applyAlignment` сначала сбрасывает float/padding, затем для float-режима ставит `float:left|right` + боковой padding на shrink-to-fit контейнер (внутренний `<img>` уже с `max-width:100%`). Резолвнутый align зеркалится в `data-image-align` на контейнере для адаптивного правила. `data-align` уже round-trip'ится через parse/renderHTML → float переживает сериализацию/коллаборацию/историю без смены схемы. - `image-menu.tsx`: кнопки Float-left / Float-right в bubble-меню (`IconFloatLeft/Right`) с active-состоянием. - `image-resize.module.css`: на узких экранах (≤600px) плавающая картинка схлопывается в 100% ширины и сбрасывает float (`!important`, по `data-image-align`) — это follow-up-коммит «100% width on small screen» из Forkmost. - i18n: en-US + ru-RU. ## Проверка `pnpm --filter @docmost/editor-ext build` + client `tsc --noEmit` — чисто. Логика и сериализация подтверждены типами/сборкой; визуальное обтекание лучше глянуть в браузере. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Ghost added 1 commit 2026-06-24 06:36:05 +03:00
feat(editor): float image with text wrap (#145, port from Forkmost) 8ef66ba712 
Adds floatLeft / floatRight image alignment so text wraps beside the image,
beyond the existing block left/center/right. Ported from Forkmost PR #7 /
upstream Docmost PR #1132 (fuscodev), adapted to gitmost's imperative image
node-view (the upstream uses a React styled component; ours styles the node-view
container directly via applyAlignment).

- editor-ext image.ts: `setImageAlign` accepts `floatLeft`/`floatRight`;
  `applyAlignment` resets float/padding then, for a float mode, sets
  `float:left|right` + side padding on the (shrink-to-fit) container so text
  flows beside it (the inner <img> already has max-width:100%). The resolved
  align is mirrored onto the container as `data-image-align` for the responsive
  rule. `data-align` already round-trips the value through parse/renderHTML, so
  float survives serialization / collab / history with no schema change.
- image-menu.tsx: Float-left / Float-right bubble-menu buttons (IconFloatLeft/
  Right) with active state.
- image-resize.module.css: on narrow screens (<=600px) a floated image collapses
  to full width and drops the float (`!important`, keyed on data-image-align) —
  the upstream "100% width on small screen" follow-up.
- i18n: en-US + ru-RU strings.

editor-ext build + client tsc --noEmit clean. Visual wrap behavior is best
confirmed in-browser (logic/serialization verified by build + types).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Ghost added 1 commit 2026-06-24 12:49:47 +03:00
fix(editor): load the float responsive rule + test applyAlignment (#145 review) 99359fa0fa 
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 <noreply@anthropic.com>
vvzvlad commented 2026-06-24 13:05:24 +03:00
Owner
Copy Link

Code review — float image with text wrap

Вердикт: Approve with comments. Прогнаны все 8 аспектов (Full‑tier, >100 строк). Критичных проблем и регрессий нет, ядро новой логики (applyAlignment) покрыто проходящими тестами (6/6, jsdom). Замечания ниже — только улучшения, мердж не блокируют.

Critical

Нет.

Warnings

Нет. Security, documentation/a11y/i18n, simplification — LGTM. Инъекций нет: align используется только в сравнениях ===, значения стилей — литералы, data-image-align пишется через DOM dataset (экранируется браузером). i18n‑политика (только en‑US + ru‑RU, fallbackLng:"en-US") задокументирована в i18n.ts. У обеих кнопок есть aria-label={t(...)}.

Suggestions

  • [conventions] Незаскоупленный :global([data-image-align=…]) — node-resize.module.css:84-90
    Единственный другой :global в файле локально заскоуплен (.container:global(.ProseMirror-selectednode), стр. 47), а новое правило полностью глобально в *.module.css. Работает только потому, что data-image-align есть лишь на image‑контейнере.
    Fix: .container:global([data-image-align="floatLeft"]), .container:global([data-image-align="floatRight"]) — это тот же элемент, на который applyAlignment пишет dataset.imageAlign.

  • [stability/regressions] Фолбэк ImageView не знает про float — image-view.tsx:13-18
    alignClass мапит только left/right/center; floatLeft/floatRight молча уходят в alignCenter. Не живая регрессия: ImageView обслуживает только плейсхолдер/загрузку (нет src) или конфиг с выключенным resize; картинка с src рендерится через float‑aware ResizableNodeView. Fix (по желанию): добавить в memo if (align === "floatLeft") return "alignLeft"; if (align === "floatRight") return "alignRight";.

  • [stability] У плавающей картинки нет clearing — node-resize.module.css:76-90
    Классика CSS‑float: картинка выше следующего короткого абзаца может перекрыть последующие блоки (картинку, код, таблицу). Присуще самой фиче, не дефект кода — стоит глазами проверить кейс «высокая картинка → короткий абзац → блок» и решить, нужен ли clear.

Test coverage

Ядро покрыто осмысленно: image.spec.ts гоняет все 5 веток applyAlignment (cssFloat/padding/dataset.imageAlign/justifyContent) и пинит reset‑guard на floatLeft→left. Тест проходит (6/6, jsdom). Один пробел:

  • [test-coverage] Ветка else/default тестируется только валидным "center" — image.spec.ts:49-56
    applyAlignment типизирована как align: string и зовётся с node.attrs.align из parseHTML(getAttribute("data-align")) — устаревшее/неизвестное значение реально попадёт в else. Контракт «любой неизвестный align безопасно деградирует в center» не запинен. Fix: добавить кейс applyAlignment(el, "justify") с проверкой justifyContent==="center", cssFloat==="", padding==="", dataset.imageAlign==="justify".

Node‑view (вызовы на init/onUpdate) и кнопки меню тестами не покрыты — но это соответствует планке проекта (у существующих кнопок left/center/right тестов тоже нет), не отмечаю.

Architecture & design (forward-looking, non-blocking)

1. Дублирование applyAlignment по image/video/drawio/excalidraw. Ветка left/center/right побайтово идентична в четырёх расширениях; PR расширяет до 5 веток только image.ts, увеличивая расхождение. Дублирование было до PR.

  • A — оставить как есть, float остаётся image‑only (small). + scope PR не раздут, ноль риска для остальных, нет спекулятивной обобщённости. − расхождение сохраняется.
  • B — вынести общий applyMediaAlignment(container, align) (medium). + единый источник, новые режимы один раз. − правка четырёх расширений в PR про image‑float, второго потребителя float пока нет.
  • Рекомендация: A. Триггер для B — момент, когда float понадобится второму типу ноды.

2. Раздвоение источника истины: data-align (renderHTML) vs data-image-align + inline‑стили (node‑view). Подтверждено: ни в клиентском CSS, ни в серверном generateHTML нет правила data-align → выравнивание. Выравнивание (и существующее left/center/right, и float) применяется только JS‑нодвью. Следствие: в серверно‑сериализованном HTML (экспорт HTML/DOCX/PDF, статика публичного шаринга) float не рендерится — как и текущее выравнивание. В read‑only редакторе внутри приложения (те же расширения, resize включён) float работает. Это не регрессия — то же ограничение у left/center/right.

  • A — принять границу, задокументировать, что выравнивание медиа только в редакторе (small). + честно, ноль риска. − float тихо деградирует в экспорте.
  • B — единый prose‑стиль (редактор + read‑only + экспорт) с data-align как единственным источником истины, убрать data-image-align (large). + консистентность везде, уходит смелл. − масштаб больше фичи, риск регрессии выравнивания.
  • Рекомендация: A для этого PR + запись ограничения; B — отдельный follow‑up (кросс‑каттинг рефактор выравнивания).

Итог: чистая, протестированная фича; логика выравнивания корректна на всех прослеженных путях, сериализация floatLeft/floatRight через data-align безопасна (потребители трактуют значение как непрозрачную строку), старый клиент деградирует в центрированный блок без потери данных. К мерджу после (опционального) учёта замечаний — в первую очередь тест‑кейс на неизвестный align и скоуп CSS‑правила.

🤖 Multi-aspect review (security · stability · conventions · documentation · regressions · test-coverage · simplification · architecture)

## Code review — float image with text wrap **Вердикт: Approve with comments.** Прогнаны все 8 аспектов (Full‑tier, >100 строк). Критичных проблем и регрессий нет, ядро новой логики (`applyAlignment`) покрыто проходящими тестами (6/6, jsdom). Замечания ниже — только улучшения, мердж не блокируют. ### Critical Нет. ### Warnings Нет. Security, documentation/a11y/i18n, simplification — LGTM. Инъекций нет: `align` используется только в сравнениях `===`, значения стилей — литералы, `data-image-align` пишется через DOM `dataset` (экранируется браузером). i18n‑политика (только en‑US + ru‑RU, `fallbackLng:"en-US"`) задокументирована в `i18n.ts`. У обеих кнопок есть `aria-label={t(...)}`. ### Suggestions - **[conventions] Незаскоупленный `:global([data-image-align=…])`** — `node-resize.module.css:84-90` Единственный другой `:global` в файле локально заскоуплен (`.container:global(.ProseMirror-selectednode)`, стр. 47), а новое правило полностью глобально в `*.module.css`. Работает только потому, что `data-image-align` есть лишь на image‑контейнере. Fix: `.container:global([data-image-align="floatLeft"]), .container:global([data-image-align="floatRight"])` — это тот же элемент, на который `applyAlignment` пишет `dataset.imageAlign`. - **[stability/regressions] Фолбэк `ImageView` не знает про float** — `image-view.tsx:13-18` `alignClass` мапит только left/right/center; `floatLeft`/`floatRight` молча уходят в `alignCenter`. **Не живая регрессия**: `ImageView` обслуживает только плейсхолдер/загрузку (нет `src`) или конфиг с выключенным resize; картинка с `src` рендерится через float‑aware `ResizableNodeView`. Fix (по желанию): добавить в memo `if (align === "floatLeft") return "alignLeft"; if (align === "floatRight") return "alignRight";`. - **[stability] У плавающей картинки нет clearing** — `node-resize.module.css:76-90` Классика CSS‑float: картинка выше следующего короткого абзаца может перекрыть последующие блоки (картинку, код, таблицу). Присуще самой фиче, не дефект кода — стоит глазами проверить кейс «высокая картинка → короткий абзац → блок» и решить, нужен ли `clear`. ### Test coverage Ядро покрыто осмысленно: `image.spec.ts` гоняет все 5 веток `applyAlignment` (cssFloat/padding/dataset.imageAlign/justifyContent) и пинит reset‑guard на floatLeft→left. Тест проходит (6/6, jsdom). Один пробел: - **[test-coverage] Ветка `else`/default тестируется только валидным `"center"`** — `image.spec.ts:49-56` `applyAlignment` типизирована как `align: string` и зовётся с `node.attrs.align` из `parseHTML(getAttribute("data-align"))` — устаревшее/неизвестное значение реально попадёт в `else`. Контракт «любой неизвестный align безопасно деградирует в center» не запинен. Fix: добавить кейс `applyAlignment(el, "justify")` с проверкой `justifyContent==="center"`, `cssFloat===""`, `padding===""`, `dataset.imageAlign==="justify"`. Node‑view (вызовы на init/onUpdate) и кнопки меню тестами не покрыты — но это соответствует планке проекта (у существующих кнопок left/center/right тестов тоже нет), не отмечаю. ### Architecture & design (forward-looking, non-blocking) **1. Дублирование `applyAlignment` по image/video/drawio/excalidraw.** Ветка left/center/right побайтово идентична в четырёх расширениях; PR расширяет до 5 веток только `image.ts`, увеличивая расхождение. Дублирование было до PR. - *A — оставить как есть, float остаётся image‑only* (small). + scope PR не раздут, ноль риска для остальных, нет спекулятивной обобщённости. − расхождение сохраняется. - *B — вынести общий `applyMediaAlignment(container, align)`* (medium). + единый источник, новые режимы один раз. − правка четырёх расширений в PR про image‑float, второго потребителя float пока нет. - **Рекомендация: A.** Триггер для B — момент, когда float понадобится второму типу ноды. **2. Раздвоение источника истины: `data-align` (renderHTML) vs `data-image-align` + inline‑стили (node‑view).** Подтверждено: ни в клиентском CSS, ни в серверном `generateHTML` нет правила `data-align → выравнивание`. Выравнивание (и существующее left/center/right, и float) применяется **только** JS‑нодвью. Следствие: в серверно‑сериализованном HTML (экспорт HTML/DOCX/PDF, статика публичного шаринга) float **не рендерится** — как и текущее выравнивание. В read‑only редакторе внутри приложения (те же расширения, resize включён) float работает. Это **не регрессия** — то же ограничение у left/center/right. - *A — принять границу, задокументировать, что выравнивание медиа только в редакторе* (small). + честно, ноль риска. − float тихо деградирует в экспорте. - *B — единый prose‑стиль (редактор + read‑only + экспорт) с `data-align` как единственным источником истины, убрать `data-image-align`* (large). + консистентность везде, уходит смелл. − масштаб больше фичи, риск регрессии выравнивания. - **Рекомендация: A** для этого PR + запись ограничения; B — отдельный follow‑up (кросс‑каттинг рефактор выравнивания). --- Итог: чистая, протестированная фича; логика выравнивания корректна на всех прослеженных путях, сериализация `floatLeft`/`floatRight` через `data-align` безопасна (потребители трактуют значение как непрозрачную строку), старый клиент деградирует в центрированный блок без потери данных. К мерджу после (опционального) учёта замечаний — в первую очередь тест‑кейс на неизвестный `align` и скоуп CSS‑правила. <sub>🤖 Multi-aspect review (security · stability · conventions · documentation · regressions · test-coverage · simplification · architecture)</sub>
vvzvlad commented 2026-06-24 13:06:46 +03:00
Owner
Copy Link

после исправления можно мержить

после исправления можно мержить
Ghost added 1 commit 2026-06-24 13:08:56 +03:00
style(editor): scope the float responsive :global to .container (#145 review) 43cf1913e0 
Per review: the file's other :global is locally scoped (.container:global(...)),
but the new float-reset media rule was fully global in a *.module.css. Scope it to
.container — the image node-view container carries BOTH the .container class and
the data-image-align attribute (same element), so behavior is unchanged while the
selector no longer leaks globally.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
vvzvlad merged commit e97024343a into develop 2026-06-24 14:04:04 +03:00
vvzvlad deleted branch feat/float-image 2026-06-24 14:04:11 +03:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
bug
Something isn't working
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 epic
Large multi-phase effort spanning many changes
 feature
New functionality request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 idea
Idea / proposal for discussion
 invalid
This doesn't seem right
 needs-human
эскалация: нужно решение человека
 question
Further information is requested
 refactor
Code cleanup / refactoring
 review/approved
в последнем ревью нет открытых blocking-находок
 review/changes-requested
последнее ревью оставило открытые blocking-находки
 review/needs
head не ревьюился (head != reviewed_head)
 security
Security / hardening issue
 status/blocked
ждёт зависимость blocked_by
 status/done
закрыто и проверено
 status/in-progress
в активной работе (мягкая заявка)
 status/ready
специфицировано, не заблокировано, ждёт исполнителя
 test
Test coverage / test infrastructure
 wontfix
This will not be worked on
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
agent_coder agent_reviewer vvzvlad
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vvzvlad/gitmost#157
Delete Branch "feat/float-image"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?