Skip to content
Merged
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
27 changes: 16 additions & 11 deletions apps/web/src/components/planreview/PlanReviewEditor.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ import {
useState,
} from "react";

import { PlanReviewCommentPopover } from "./plate/PlanReviewCommentPopover";
import { PlanReviewCommentComposer } from "./plate/PlanReviewCommentComposer";
import {
PLAN_REVIEW_QUICK_LABELS,
PlanReviewFloatingToolbar,
Expand Down Expand Up @@ -576,17 +576,22 @@ function PlanReviewEditorImpl({
readOnly={readOnly}
hidden={pendingQuote !== null}
/>
{pendingQuote !== null ? (
<PlanReviewCommentPopover
containerRef={surfaceRef}
quotedText={pendingQuote}
body={commentBody}
onBodyChange={setCommentBody}
onSubmit={submitComment}
onCancel={cancelComment}
/>
) : null}
</div>

{/*
Outside the scroll container on purpose: as a child it scrolled the plan
when it took focus, which threw the reviewer back to the top of a long
document the moment they started writing.
*/}
{pendingQuote !== null ? (
<PlanReviewCommentComposer
quotedText={pendingQuote}
body={commentBody}
onBodyChange={setCommentBody}
onSubmit={submitComment}
onCancel={cancelComment}
/>
) : null}
</div>
);
}
Expand Down
19 changes: 9 additions & 10 deletions apps/web/src/components/planreview/PlanReviewPanel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -523,14 +523,12 @@ export default function PlanReviewPanel({
</p>
</>
) : (
<div className="flex items-center gap-1.5">
/* Stacked, not side by side: the rail is 288px and
"Approve with comments" was being cut in half by a shared row. */
<div className="flex flex-col gap-1.5">
{hasFeedbackToSend ? (
<Button
size="sm"
className="flex-1"
onClick={() => handleSubmit("changes-requested")}
>
<SendIcon className="size-3.5" aria-hidden /> Send feedback
<Button size="sm" onClick={() => handleSubmit("changes-requested")}>
<SendIcon className="size-3.5 shrink-0" aria-hidden /> Send feedback
{openDiscussionCount > 0 ? (
<span className="ml-1 rounded-full bg-primary-foreground/20 px-1.5 text-[11px] tabular-nums">
{openDiscussionCount}
Expand All @@ -541,16 +539,17 @@ export default function PlanReviewPanel({
<Button
size="sm"
variant={hasFeedbackToSend ? "outline" : "default"}
className="flex-1"
onClick={() => handleSubmit("approved")}
title={
openDiscussionCount > 0
? "Start implementing, with your open comments sent as refinements"
: undefined
}
>
<CheckIcon className="size-3.5" aria-hidden />{" "}
{openDiscussionCount > 0 ? "Approve with comments" : "Approve"}
<CheckIcon className="size-3.5 shrink-0" aria-hidden />
<span className="truncate">
{openDiscussionCount > 0 ? "Approve with comments" : "Approve"}
</span>
</Button>
</div>
)}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
import { renderToStaticMarkup } from "react-dom/server";
import { describe, expect, it } from "vite-plus/test";

import { PlanReviewCommentComposer } from "./PlanReviewCommentComposer";

function render(body: string, quotedText = "Backfill the rows") {
return renderToStaticMarkup(
<PlanReviewCommentComposer
quotedText={quotedText}
body={body}
onBodyChange={() => {}}
onSubmit={() => {}}
onCancel={() => {}}
/>,
);
}

describe("PlanReviewCommentComposer", () => {
/**
* The composer used to float inside the scrolling document. Focusing it
* scrolled the plan, throwing the reviewer from wherever they were reading
* back to the top. It docks now, and must not regress to positioned layout.
*/
it("is docked, not positioned over the document", () => {
// Asserted on the root element's own classes: the button utilities below it
// legitimately mention `absolute`, so a whole-markup search would lie.
const rootClass = /^<div class="([^"]*)"/.exec(render(""))?.[1] ?? "";

expect(rootClass).toContain("border-t");
expect(rootClass).not.toContain("absolute");
expect(rootClass).not.toContain("fixed");
expect(rootClass).not.toContain("sticky");
});

it("is big enough to write in and capped so it cannot swallow the plan", () => {
const markup = render("");

expect(markup).toContain("min-height:6rem");
expect(markup).toContain("max-height:30vh");
});

it("shows the quoted anchor and the keyboard shortcuts", () => {
const markup = render("", "Ticket: new prospect threads");

expect(markup).toContain("Ticket: new prospect threads");
expect(markup).toContain("Esc to cancel");
expect(markup).toContain("Enter to comment");
});

it("cannot submit an empty comment", () => {
// The rendered attribute, not the `disabled:` utility classes every button carries.
expect(render("")).toContain('disabled=""');
expect(render("Batch this.")).not.toContain('disabled=""');
expect(render(" ")).toContain('disabled=""');
});
});
116 changes: 116 additions & 0 deletions apps/web/src/components/planreview/plate/PlanReviewCommentComposer.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
/**
* T3-CUSTOM(expbkt3): the comment composer, docked under the plan.
*
* This was briefly a popover anchored to the selection. That was wrong twice
* over: it lived *inside* the scrolling document, so autofocusing it scrolled
* the plan — from the bottom of a long plan, back to the top — and the box was
* too small to write a real review comment in.
*
* Docking it below the document fixes both by construction. It is a sibling of
* the scroll container rather than a child, so focusing it cannot move the plan,
* and it has the full width of the column to grow into.
*
* It grows with what is typed, up to `MAX_HEIGHT_VH` of the viewport — long
* enough to read a paragraph back before sending it, bounded so the composer can
* never swallow the plan it is about.
*/
import { useEffect, useLayoutEffect, useRef } from "react";

import { Button } from "../../ui/button";

/** Enough for ~5 lines, so the box reads as somewhere to write prose. */
const MIN_HEIGHT_REM = 6;
const MAX_HEIGHT_VH = 30;

const IS_APPLE = typeof navigator !== "undefined" && /Mac|iPhone|iPad/.test(navigator.userAgent);

export function PlanReviewCommentComposer({
quotedText,
body,
onBodyChange,
onSubmit,
onCancel,
}: {
readonly quotedText: string;
readonly body: string;
readonly onBodyChange: (body: string) => void;
readonly onSubmit: () => void;
readonly onCancel: () => void;
}) {
const inputRef = useRef<HTMLTextAreaElement | null>(null);

useEffect(() => {
// `preventScroll` because the plan is a scroll container and the reviewer is
// usually reading some way down it. Belt and braces alongside docking: even
// a stray ancestor scroll would undo the thing this component exists to fix.
inputRef.current?.focus({ preventScroll: true });
}, []);

// Grow to fit the text, then stop. Measured rather than done with CSS
// `field-sizing`, which Safari and Firefox still do not support.
useLayoutEffect(() => {
const input = inputRef.current;
if (input === null) return;
input.style.height = "auto";
const maxHeight = (window.innerHeight * MAX_HEIGHT_VH) / 100;
input.style.height = `${Math.min(input.scrollHeight, maxHeight)}px`;
}, [body]);

const canSubmit = body.trim().length > 0;

return (
<div className="shrink-0 border-t bg-card">
<div className="flex items-start gap-2 px-3 pt-2">
<blockquote
className="min-w-0 flex-1 border-amber-400/60 border-l-2 pl-2 text-muted-foreground text-xs italic"
title={quotedText}
>
<span className="line-clamp-2 whitespace-pre-wrap wrap-break-word">{quotedText}</span>
</blockquote>
<button
type="button"
aria-label="Cancel comment"
className="shrink-0 rounded px-1 text-muted-foreground text-xs transition-colors hover:bg-accent hover:text-foreground"
onClick={onCancel}
>
✕
</button>
</div>

<div className="px-3 pt-2">
<textarea
ref={inputRef}
className="w-full resize-none overflow-y-auto rounded-md border bg-background p-2 text-sm outline-none focus-visible:ring-1 focus-visible:ring-ring"
style={{ minHeight: `${MIN_HEIGHT_REM}rem`, maxHeight: `${MAX_HEIGHT_VH}vh` }}
value={body}
placeholder="What should change here?"
aria-label="Comment body"
onChange={(event) => onBodyChange(event.target.value)}
onKeyDown={(event) => {
if (event.key === "Escape") {
event.preventDefault();
onCancel();
return;
}
if (event.key === "Enter" && (event.metaKey || event.ctrlKey)) {
event.preventDefault();
if (canSubmit) onSubmit();
}
}}
/>
</div>

<div className="flex items-center justify-end gap-2 px-3 py-2">
<span aria-hidden className="mr-auto text-[11px] text-muted-foreground">
{IS_APPLE ? "⌘" : "Ctrl"}+Enter to comment · Esc to cancel
</span>
<Button size="sm" variant="ghost" onClick={onCancel}>
Cancel
</Button>
<Button size="sm" onClick={onSubmit} disabled={!canSubmit}>
Comment
</Button>
</div>
</div>
);
}
106 changes: 0 additions & 106 deletions apps/web/src/components/planreview/plate/PlanReviewCommentPopover.tsx

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -110,10 +110,10 @@ export function PlanReviewFloatingToolbar({
readonly onComment: () => void;
readonly onQuickLabel: (label: PlanReviewQuickLabel) => void;
readonly readOnly: boolean;
/** Suppressed while the comment popover owns the selection. */
/** Suppressed while the comment composer owns the selection. */
readonly hidden?: boolean;
}) {
const anchor = useSelectionAnchor({ containerRef, frozen: hidden });
const anchor = useSelectionAnchor({ containerRef });

if (hidden || anchor === null) return null;

Expand Down
Loading
Loading