Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
87 changes: 77 additions & 10 deletions src/components/Editor.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import { useState, useCallback } from "react";
import { useEditor, EditorContent } from "@tiptap/react";
import StarterKit from "@tiptap/starter-kit";
import Placeholder from "@tiptap/extension-placeholder";
import { TextStyle, FontFamily, FontSize, Color } from "@tiptap/extension-text-style";
import {
TextStyle,
FontFamily,
FontSize,
Color,
} from "@tiptap/extension-text-style";
import Highlight from "@tiptap/extension-highlight";
import TextAlign from "@tiptap/extension-text-align";
import Link from "@tiptap/extension-link";
Expand All @@ -11,30 +17,91 @@ import Toolbar from "./Toolbar";
import PageView from "./PageView";

function Editor() {
const editor = useEditor({
const [isHeaderVisible, setIsHeaderVisible] = useState(false);
const [activeEditorType, setActiveEditorType] = useState<"body" | "header">(
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

"body"
);
const [headerHtml, setHeaderHtml] = useState("");
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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


const bodyEditor = useEditor({
extensions: [
StarterKit.configure({
heading: { levels: [1, 2, 3, 4] },
}),
Link.configure({ openOnClick: false }),
Underline,
TextStyle,
FontFamily,
FontSize,
Color,
Underline.configure(),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Code Quality] 🟠 src/components/Editor.tsx:32 and src/components/Editor.tsx:58 duplicate almost the entire TipTap extension setup for the body and header editors. That means every future formatting change has to be kept in sync across two arrays with slightly different overrides, which is exactly the kind of drift-prone configuration that becomes hard to maintain. A small factory/helper for the shared extensions would keep the divergence explicit.

TextStyle.configure(),
FontFamily.configure(),
FontSize.configure(),
Color.configure(),
Highlight.configure({ multicolor: true }),
TextAlign.configure({ types: ["heading", "paragraph"] }),
Image.configure({ inline: true }),
Placeholder.configure({ placeholder: "Start typing..." }),
],
autofocus: true,
onFocus: () => setActiveEditorType("body"),
});

const headerEditor = useEditor({
extensions: [
StarterKit.configure({
heading: false,
bulletList: false,
orderedList: false,
listItem: false,
codeBlock: false,
horizontalRule: false,
blockquote: false,
}),
Link.configure({ openOnClick: false }),
Underline.configure(),
TextStyle.configure(),
FontFamily.configure(),
FontSize.configure(),
Color.configure(),
Highlight.configure({ multicolor: true }),
TextAlign.configure({ types: ["paragraph"] }),
Placeholder.configure({ placeholder: "Click here to add a header" }),
],
onFocus: () => setActiveEditorType("header"),
onUpdate: ({ editor }) => {
setHeaderHtml(editor.getHTML());
},
});

const activeEditor =
activeEditorType === "header" && headerEditor ? headerEditor : bodyEditor;

const toggleHeader = useCallback(() => {
setIsHeaderVisible((previous) => {
if (previous && activeEditorType === "header") {
setActiveEditorType("body");
bodyEditor?.commands.focus();
}
return !previous;
});
}, [activeEditorType, bodyEditor]);

return (
<>
{editor && <Toolbar editor={editor} />}
<PageView>
<EditorContent editor={editor} className="h-full" />
{activeEditor && (
<Toolbar
editor={activeEditor}
isHeaderVisible={isHeaderVisible}
onToggleHeader={toggleHeader}
/>
)}
<PageView
documentHeaderSlot={
headerEditor && (
<EditorContent editor={headerEditor} className="header-editor" />
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
<EditorContent editor={headerEditor} className="header-editor" />
<EditorContent
editor={headerEditor}
className="header-editor"
aria-label="Document header"
/>

)
}
Comment on lines +96 to +100
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not use ternary operator.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 5ce6dc9 — replaced the ternary with a logical AND (headerEditor && <EditorContent ... />).

documentHeaderHtml={headerHtml}
isDocumentHeaderVisible={isHeaderVisible}
>
<EditorContent editor={bodyEditor} className="h-full" />
</PageView>
</>
);
Expand Down
128 changes: 106 additions & 22 deletions src/components/PageView.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,18 @@
import { useRef, useState, useEffect, useCallback, type ReactNode } from "react";
import {
useRef,
useState,
useEffect,
useCallback,
type ReactNode,
} from "react";

const PAGE_WIDTH = 816;
const HEADER_HEIGHT = 48;
const FOOTER_HEIGHT = 48;
const GAP_HEIGHT = 40;
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;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.


const PAGE_SHADOW = "0 1px 3px rgba(0,0,0,0.12), 0 1px 2px rgba(0,0,0,0.08)";

Expand All @@ -18,14 +24,20 @@ const pageNumberStyle = {

interface PageViewProps {
children: ReactNode;
documentHeaderSlot?: ReactNode;
documentHeaderHtml?: string;
isDocumentHeaderVisible?: boolean;
}

/**
* Finds the .tiptap element inside the content ref and adds margin-bottom
* Finds the .tiptap element inside the content ref and adds margin-top
* to block elements that would straddle a page boundary, pushing them to
* the next page. Returns the total number of pages.
*/
function adjustBlockMarginsForPageBreaks(contentElement: HTMLElement): number {
function adjustBlockMarginsForPageBreaks(
contentElement: HTMLElement,
overlayHeight: number
): number {
const tiptapElement = contentElement.querySelector(".tiptap");
if (!tiptapElement) return 1;

Expand All @@ -47,17 +59,17 @@ function adjustBlockMarginsForPageBreaks(contentElement: HTMLElement): number {
const blockTop = blockRect.top - contentTop;
const blockBottom = blockRect.bottom - contentTop;

// Account for margins already added (each previous page break adds OVERLAY_HEIGHT)
// Account for margins already added (each previous page break adds overlayHeight)
const previousBreaks = currentPage - 1;
const adjustedTop = blockTop - previousBreaks * OVERLAY_HEIGHT;
const adjustedBottom = blockBottom - previousBreaks * OVERLAY_HEIGHT;
const adjustedTop = blockTop - previousBreaks * overlayHeight;
const adjustedBottom = blockBottom - previousBreaks * overlayHeight;

const pageBoundary = currentPage * PAGE_CONTENT_HEIGHT;

// If this block crosses a page boundary
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;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

block.style.marginTop = `${spaceNeeded}px`;
block.dataset.pageMargin = "true";
currentPage++;
Expand All @@ -70,27 +82,59 @@ function adjustBlockMarginsForPageBreaks(contentElement: HTMLElement): number {
// Calculate total pages from the final content height
const finalHeight = tiptapElement.getBoundingClientRect().height;
const totalBreaks = currentPage - 1;
const adjustedHeight = finalHeight - totalBreaks * OVERLAY_HEIGHT;
const adjustedHeight = finalHeight - totalBreaks * overlayHeight;
return Math.max(1, Math.ceil(adjustedHeight / PAGE_CONTENT_HEIGHT));
}

function PageView({ children }: PageViewProps) {
function PageView({
children,
documentHeaderSlot,
documentHeaderHtml,
isDocumentHeaderVisible = false,
}: PageViewProps) {
const contentRef = useRef<HTMLDivElement>(null);
const headerMeasureRef = useRef<HTMLDivElement>(null);
const [totalPages, setTotalPages] = useState(1);
const [documentHeaderRenderedHeight, setDocumentHeaderRenderedHeight] =
useState(0);
const isAdjustingRef = useRef(false);

const showHeader = isDocumentHeaderVisible && !!documentHeaderSlot;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

const headerHeight = showHeader ? documentHeaderRenderedHeight : 0;
const effectiveOverlayHeight = BASE_OVERLAY_HEIGHT + headerHeight;

// Measure document header height
useEffect(() => {
const element = headerMeasureRef.current;
if (!element || !showHeader) {
setDocumentHeaderRenderedHeight(0);
return;
}
const measureHeight = () => {
const height = element.getBoundingClientRect().height;
setDocumentHeaderRenderedHeight(height);
};
measureHeight();
const observer = new ResizeObserver(measureHeight);
observer.observe(element);
return () => observer.disconnect();
}, [showHeader]);

const recalculatePages = useCallback(() => {
if (isAdjustingRef.current) return;
const element = contentRef.current;
if (!element) return;

isAdjustingRef.current = true;
const pages = adjustBlockMarginsForPageBreaks(element);
const pages = adjustBlockMarginsForPageBreaks(
element,
effectiveOverlayHeight
);
if (pages !== totalPages) {
setTotalPages(pages);
}
isAdjustingRef.current = false;
}, [totalPages]);
}, [totalPages, effectiveOverlayHeight]);

useEffect(() => {
const element = contentRef.current;
Expand All @@ -108,11 +152,13 @@ function PageView({ children }: PageViewProps) {
return () => observer.disconnect();
}, [recalculatePages]);

// Recalculate when header height changes
useEffect(() => {
recalculatePages();
}, [headerHeight, recalculatePages]);

return (
<div
className="relative mt-4"
style={{ width: PAGE_WIDTH }}
>
<div className="relative mt-4" style={{ width: PAGE_WIDTH }}>
{/* First page header */}
<div
className="bg-white"
Expand All @@ -125,6 +171,25 @@ function PageView({ children }: PageViewProps) {
}}
/>

{/* Document header (editable, first page) */}
{showHeader && (
<div
ref={headerMeasureRef}
className="bg-white"
style={{
paddingLeft: PAGE_HORIZONTAL_PADDING,
paddingRight: PAGE_HORIZONTAL_PADDING,
boxShadow: PAGE_SHADOW,
clipPath: "inset(0 -10px)",
}}
>
<div className="document-header bg-[#f8f9fc] rounded-sm px-3 py-2">
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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

{documentHeaderSlot}
</div>
<div className="border-b border-gray-300 mt-2" />
</div>
)}

{/* Content area */}
<div
ref={contentRef}
Expand Down Expand Up @@ -160,11 +225,12 @@ function PageView({ children }: PageViewProps) {
{/* Page gap overlays at each boundary */}
{Array.from({ length: totalPages - 1 }, (_, index) => {
const pageNumber = index + 1;
// Each overlay sits at: header + N page-content-heights + previous overlays
// Each overlay sits at: first page header + document header + N page-content-heights + previous overlays
const topPosition =
HEADER_HEIGHT +
headerHeight +
PAGE_CONTENT_HEIGHT * pageNumber +
index * OVERLAY_HEIGHT;
index * effectiveOverlayHeight;

return (
<div
Expand All @@ -190,10 +256,7 @@ function PageView({ children }: PageViewProps) {
</div>

{/* Gap between pages */}
<div
className="bg-[#F8F9FA]"
style={{ height: GAP_HEIGHT }}
/>
<div className="bg-[#F8F9FA]" style={{ height: GAP_HEIGHT }} />

{/* Header of next page */}
<div
Expand All @@ -206,6 +269,27 @@ function PageView({ children }: PageViewProps) {
borderTopRightRadius: 4,
}}
/>

{/* Document header (read-only copy for subsequent pages) */}
{showHeader && documentHeaderHtml && (
<div
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Suggested change
<div
<div
aria-hidden="true"

className="bg-white"
style={{
paddingLeft: PAGE_HORIZONTAL_PADDING,
paddingRight: PAGE_HORIZONTAL_PADDING,
boxShadow: PAGE_SHADOW,
clipPath: "inset(0 -10px)",
}}
>
<div className="document-header bg-[#f8f9fc] rounded-sm px-3 py-2">
<div
className="tiptap"
dangerouslySetInnerHTML={{ __html: documentHeaderHtml }}
/>
</div>
<div className="border-b border-gray-300 mt-2" />
</div>
)}
</div>
);
})}
Expand Down
Loading