feat(editor): add document header with rich text editing and per-page display#3
feat(editor): add document header with rich text editing and per-page display#3ranjan-codewalnut wants to merge 2 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
- Add persistent header area that appears on every page with visual pagination - Support rich text editing (bold, italic, underline, font, color) in header - Show read-only header copy on subsequent pages for print-like appearance - Add toolbar toggle button to show/hide the header section - Store header content separately from main document body - Add visual separator between header and document content - Disable unsupported toolbar features (lists, headings, images, HR) when header editor is focused to prevent crashes - Fix duplicate TipTap extension warning by creating separate instances Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ranjan-codewalnut
force-pushed
the
feat/document-header
branch
from
4c4f7eb to
ef4b2ba
Compare
| documentHeaderSlot={ | ||
| headerEditor ? ( | ||
| <EditorContent editor={headerEditor} className="header-editor" /> | ||
| ) : null | ||
| } |
There was a problem hiding this comment.
Do not use ternary operator.
There was a problem hiding this comment.
Fixed in 5ce6dc9 — replaced the ternary with a logical AND (headerEditor && <EditorContent ... />).
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ranjan-codewalnut
force-pushed
the
feat/document-header
branch
from
752c245 to
5ce6dc9
Compare
|
@mergemitra review |
PR ReviewScoresPR Quality NotesPR Description & Scope
Commit Messages & History
PR Size & Focus
Self-Review & Polish
TL;DRThis PR adds an editable document header, a toolbar toggle, and repeated per-page header rendering with schema-aware toolbar gating for the header editor. The main risks are in page geometry: pagination still treats the header as extra chrome instead of reducing body capacity, and stale page-break margins can survive header toggles or resizes. Accessibility and verification also lag behind the new UI because the header toggle is mouse-only, repeated header clones stay in the accessibility tree, and the repo still has no automated test infrastructure. Merge Recommendation: Approve with suggestions — the feature is close, but pagination correctness and accessibility regressions should be addressed before relying on the header in real multi-page documents. Focus Areas for Architect Review
Based on 5b0ad11...5ce6dc9 |
![mergemitra[bot]](https://avatars.githubusercontent.com/in/2493799?s=60&v=4){kind=small}
| const PAGE_CONTENT_HEIGHT = 1056 - HEADER_HEIGHT - FOOTER_HEIGHT; | ||
| const PAGE_HORIZONTAL_PADDING = 96; | ||
| const OVERLAY_HEIGHT = HEADER_HEIGHT + FOOTER_HEIGHT + GAP_HEIGHT; | ||
| const BASE_OVERLAY_HEIGHT = HEADER_HEIGHT + FOOTER_HEIGHT + GAP_HEIGHT; |
There was a problem hiding this comment.
[Correctness] 🟠 In src/components/PageView.tsx:13-15, src/components/PageView.tsx:103-105, src/components/PageView.tsx:174-205, and src/components/PageView.tsx:229-233, the new document header is added on top of the existing 960px body area instead of reducing the per-page body capacity. Once the header has any height, each page becomes 1056 + headerHeight tall, but the break logic still allows the full 960px of body content on that page. In practice, any document that was near a page boundary before turning the header on will paginate too late, which contradicts the PR’s “print-like pagination” goal.
| if (adjustedTop < pageBoundary && adjustedBottom > pageBoundary) { | ||
| // Add margin to push it to the next page's content area | ||
| const spaceNeeded = pageBoundary - adjustedTop + OVERLAY_HEIGHT; | ||
| const spaceNeeded = pageBoundary - adjustedTop + overlayHeight; |
There was a problem hiding this comment.
[Correctness] 🟠 src/components/PageView.tsx:47-52 clears marginBottom, but the page-break spacer is written to marginTop at src/components/PageView.tsx:72-74. That means the new header toggle / header height observer can never fully recompute pagination: if a block needed a forced break before the header was hidden or resized, its old top margin stays in the layout even when that break is no longer needed, so content remains pushed onto later pages and totalPages can stay inflated after toggling the header.
if (block.dataset.pageMargin) {
block.style.marginTop = "";
delete block.dataset.pageMargin;
}ℹ️ Original location:
src/components/PageView.tsx:52— moved to nearest diff line for GitHub compatibility.
| const [activeEditorType, setActiveEditorType] = useState<"body" | "header">( | ||
| "body" | ||
| ); | ||
| const [headerHtml, setHeaderHtml] = useState(""); |
There was a problem hiding this comment.
[Correctness] 🟠 src/components/Editor.tsx:24,68-70 initializes headerHtml as an empty string and only populates it after the header editor emits an update, while src/components/PageView.tsx:274-291 only renders repeated headers when documentHeaderHtml is truthy. If the user turns the header on and immediately starts writing body content, page 1 shows the editable header but page 2+ render no repeated header at all, even though the pagination math is already reserving that vertical space. That breaks the advertised “header appears on every page” behavior for untouched headers.
Related: src/components/PageView.tsx:274-291
| <ToolbarDivider /> | ||
|
|
||
| {/* Toggle Document Header */} | ||
| <ToolbarButton |
There was a problem hiding this comment.
[Enterprise Quality] 🟠 The new header toggle is not keyboard-operable. ToolbarButton only dispatches actions from onMouseDown, and the new PanelTop control uses that component, so Tab + Enter / Space never shows or hides the header. That makes the feature inaccessible to keyboard-only users.
<button
type="button"
title={title}
disabled={disabled}
onMouseDown={(event) => {
event.preventDefault();
onClick();
}}
onKeyDown={(event) => {
if (event.key === "Enter" || event.key === " ") {
event.preventDefault();
onClick();
}
}}
>ℹ️ Original location:
src/components/Toolbar.tsx:79— moved to nearest diff line for GitHub compatibility.
|
|
||
| {/* Document header (read-only copy for subsequent pages) */} | ||
| {showHeader && documentHeaderHtml && ( | ||
| <div |
There was a problem hiding this comment.
[Enterprise Quality] 🟠 The per-page header clones are visual-only, but they are still exposed to assistive technology. On a multi-page document, screen readers will encounter the same header content once for the editable first-page header and then again for every subsequent page clone, which makes navigation noisy and misleading. These repeated copies should be hidden from the accessibility tree.
| <div | |
| <div | |
| aria-hidden="true" |
| clipPath: "inset(0 -10px)", | ||
| }} | ||
| > | ||
| <div className="document-header bg-[#f8f9fc] rounded-sm px-3 py-2"> |
There was a problem hiding this comment.
[Code Quality] 🟠 The document-header shell is duplicated almost verbatim in the first-page branch at src/components/PageView.tsx:186 and the repeated-page branch at src/components/PageView.tsx:284, with the same divider duplicated again at src/components/PageView.tsx:189 and src/components/PageView.tsx:290. Styling or spacing changes now require touching both branches, which makes page-1 and page-N behavior easy to let drift apart. Extracting a small wrapper component/helper would remove that duplication.
[Code Quality] 💬 Minor [nitpick] The new header styling introduces raw color literals in multiple places: src/components/PageView.tsx:186, src/components/PageView.tsx:189, src/index.css:70, and src/index.css:76. Those values now form a separate mini-theme for the header, which makes later restyling harder than using shared CSS variables or named design tokens.
Related: src/index.css:70-76
| <PageView | ||
| documentHeaderSlot={ | ||
| headerEditor && ( | ||
| <EditorContent editor={headerEditor} className="header-editor" /> |
There was a problem hiding this comment.
[Enterprise Quality] 💬 Minor [nitpick] The new header editor adds a second editable region without a programmatic name, so assistive-tech users get another generic editable surface with no cue that it is the document header rather than the body. Adding an explicit label would make the two editors distinguishable.
| <EditorContent editor={headerEditor} className="header-editor" /> | |
| <EditorContent | |
| editor={headerEditor} | |
| className="header-editor" | |
| aria-label="Document header" | |
| /> |
| useState(0); | ||
| const isAdjustingRef = useRef(false); | ||
|
|
||
| const showHeader = isDocumentHeaderVisible && !!documentHeaderSlot; |
There was a problem hiding this comment.
[Code Quality] 💬 Minor [nitpick] showHeader in src/components/PageView.tsx:102 is a boolean without an is/has/should prefix. A name like isHeaderShown or shouldShowHeader reads more clearly in conditions and stays aligned with the project’s boolean naming rule.
| function Editor() { | ||
| const editor = useEditor({ | ||
| const [isHeaderVisible, setIsHeaderVisible] = useState(false); | ||
| const [activeEditorType, setActiveEditorType] = useState<"body" | "header">( |
There was a problem hiding this comment.
[Code Quality] 💬 Minor [nitpick] src/components/Editor.tsx:21, src/components/Editor.tsx:43, src/components/Editor.tsx:67, and src/components/Editor.tsx:78 repeat the editor-mode literals "body" and "header" across state, focus handlers, and toggle logic. Centralizing those values behind a small constant or enum would make this state machine less brittle if another editor mode is added later.
| } | ||
|
|
||
| /* Document header editor styles */ | ||
| .document-header .tiptap { |
There was a problem hiding this comment.
[Code Quality] 💬 Minor [nitpick] src/index.css:65 and src/index.css:73 re-declare the base .tiptap and placeholder selectors almost line-for-line just to tweak the header variant. That duplicates editor typography behavior in two places, so future changes to placeholder styling or base text rules will need manual syncing. A modifier class or CSS custom properties layered on top of .tiptap would be easier to maintain.
1 participant
PR Template & Definition of Done
What does this PR do?
There is no node type named 'listItem') caused by callingeditor.can().liftListItem("listItem")during render when the header editor's schema doesn't include list nodes..configure()to create separate extension instances for each editor.PageViewto account for the dynamic document header height using aResizeObserver.What steps does your reviewer have to take to test this PR manually?
bun devand open the editor in the browser.listItemcrash).Pull Request standards checklist - Please check off
Testing checklist - Please check off
If you have not followed and completed any of the above, please explain why below.
...
Definition of Done - Please check off