feat: support rendering of line between columns#2743
feat: support rendering of line between columns#2743Abdeltoto wants to merge 2 commits intosuperdoc-dev:mainfrom
Conversation
Parse the w:sep attribute and child element on w:cols in section properties to detect the "line between" column setting. Carry the lineBetween flag through ColumnLayout, SectionBreakBlock, and Page types so the DomPainter can render vertical separator lines in the inter-column gaps. Closes superdoc-dev#2067 Made-with: Cursor
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb1d82966f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (activeColumns && activeColumns.count > 1) { | ||
| page.columns = activeColumns; |
There was a problem hiding this comment.
page.columns = activeColumns assumes the new lineBetween flag survives section-state cloning, but cloneColumnLayout currently drops that field (packages/layout-engine/contracts/src/column-layout.ts, function cloneColumnLayout). As a result, page.columns.lineBetween is typically undefined even when <w:cols w:sep="1"> is parsed, so the separator rendering path never activates. Please propagate lineBetween through the column clone/state pipeline before assigning it to the page.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 42aea30. Both cloneColumnLayout and normalizeColumnLayout now propagate the field, so page.columns.lineBetween survives the section-state pipeline.
| // Render vertical separator lines between columns when lineBetween is enabled | ||
| if (page.columns?.lineBetween && page.columns.count > 1 && page.margins && this.doc) { | ||
| const cols = page.columns; |
There was a problem hiding this comment.
This separator logic was added only to renderPage, but the default vertical/non-book flow renders pages via fullRender/patchLayout/virtualization, which build DOM through createPageState instead of renderPage (same file). In those modes, separators are never created, so users in standard vertical mode won’t see the new line-between-columns feature. The separator rendering needs to be shared with or duplicated in the createPageState path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed — also fixed in 42aea30. Pulled the rendering into a shared renderColumnSeparators helper and call it from both renderPage and createPageState, so the separators show up in the default vertical/virtualized flow too.
… in vertical flow Two issues from the Codex review on superdoc-dev#2743: 1. cloneColumnLayout and normalizeColumnLayout dropped the lineBetween field, so page.columns.lineBetween was undefined by the time the renderer checked it. Both helpers now propagate the flag. 2. The separator-rendering logic only ran inside renderPage, used by the book/horizontal flow. The default vertical/virtualized flow builds pages through createPageState, so users in standard mode never saw the separators. Extracted the rendering into a shared renderColumnSeparators helper and called it from both code paths. Closes both Codex P1 findings on the PR. Made-with: Cursor
2 participants
Summary
w:sepattribute and child element onw:colsin section properties to detect the line-between column settinglineBetweenflag toColumnLayout,SectionBreakBlock.columns, andPagetypes in contractslineBetweenthrough the pm-adapter extraction and layout engine page creationlineBetweenis enabledFiles changed
contracts/src/index.ts--lineBetweenonColumnLayout,SectionBreakBlock,Pagesuper-converter/section-properties.js-- parsew:sepattribute and child elementpm-adapter/src/sections/extraction.ts-- extractlineBetweenfromw:colslayout-engine/src/index.ts-- carryactiveColumnsto pagepainters/dom/src/renderer.ts-- render vertical lines in inter-column gapsTest plan
Closes #2067