fix(ui): move IssueRow divider and hover wash to the row root (#10702)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The Inbox and Tasks screens render issues as a vertical list of `IssueRow` components > - A recent refactor split `IssueRow` into a root `div` plus a full-row overlay `Link`, and the divider and hover styles moved onto that overlay > - As a result every row shows a bottom border and hover greys the text instead of tinting the background > - This pull request moves the divider and hover/selected wash back onto the root row band and keeps only positioning on the overlay > - The benefit is the list reads cleanly again: no stray dividers, and hover tints the background behind the text ## Linked Issues or Issue Description No public GitHub issue exists. Describing the bug in-PR (bug report): **What happened** - In the Inbox and Tasks list views, every row shows a 1px bottom border, including the last row. - Hovering a row dims/greys the row text instead of showing a background tint behind the content. **Expected behavior** - List rows in Inbox and Tasks show no separator lines by default. - Hover shows a subtle background tint behind the row content; the text stays fully legible. - The blocked inbox view keeps its intentional separators. **Steps to reproduce** 1. Open the Inbox or Tasks list view. 2. Note the horizontal border under every row, including the last. 3. Hover a row and note the text greys out rather than the background tinting. **Root cause** - PR #10526 restructured `IssueRow` from a single root `Link` into a root `div` plus a full-row `absolute inset-0` overlay `Link` (to keep header controls clickable). The divider and hover/selected/checklist background classes moved onto the overlay `Link`. `last:border-b-0` no longer matched (the Link is the first child of a multi-child div), and the hover wash painted on top of the content instead of behind it. **Paperclip version/commit** - Base commit: `8b83d69e3` (branched from current `master`). **Deployment mode** - UI (web) list views: Inbox and Tasks. ## What Changed - `ui/src/components/IssueRow.tsx`: moved the divider classes and the hover/selected/checklist background wash from the overlay `Link` to the root row `div`, so the tint paints behind the content and `last:border-b-0` matches the real last row. The overlay `Link` now keeps only `absolute inset-0` positioning and the focus ring. Renamed the `hideDivider` prop to an opt-in `showDivider` (default `false`). Kept `[&_button]:relative [&_button]:z-10` on the root so the Archive button stays clickable above the overlay, and kept the `isArchiving` collapse animation on the root row. - `ui/src/components/IssuesList.tsx`: dropped the old `hideDivider` usage (dividers are now opt-in). - `ui/src/pages/Inbox.tsx`: dropped the old `hideDivider` usage. - `ui/src/components/BlockedInboxView.tsx`: added `showDivider` so this view keeps its separators. ## Verification - `cd ui && npx tsc -b` — typecheck passes with the change. - Manual (recommended for reviewer): in the Inbox and Tasks list views, confirm no per-row bottom border and that the last row has none. Because dark-mode `--border` is 10% white and near-invisible in screenshots, assert the computed `border-bottom-width` on a row element rather than eyeballing pixels. - Hover a row: text stays legible; a background tint appears behind the content. - Inbox: the Archive button appears on hover and is clickable (the overlay does not swallow the click). - Blocked inbox view: separators still render. ## Risks - Low risk. The change relocates existing Tailwind classes between two elements of the same row and renames one internal prop; no data or API surface changes. All `IssueRow` call sites were updated in this PR (verified: no remaining `hideDivider` references). ## Model Used - Claude, Opus 4.8 (`claude-opus-4-8`), extended thinking with tool use. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [ ] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [ ] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [ ] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
scotttong
GitHub
parent
8b83d69e3c
commit
772fa98393
|
|
@ -359,6 +359,7 @@ function BlockedInboxRow({
|
|||
<IssueRow
|
||||
issue={row.issue}
|
||||
issueLinkState={issueLinkState}
|
||||
showDivider
|
||||
desktopMetaLeading={
|
||||
<BlockedRowDesktopMeta
|
||||
row={row}
|
||||
|
|
|
|||
|
|
@ -150,10 +150,16 @@ describe("IssueRow", () => {
|
|||
root.render(<IssueRow issue={issue} selected />);
|
||||
});
|
||||
|
||||
// The hover wash lives on the ROOT row band (not the overlay link) so the
|
||||
// tint paints behind the content. Selected rows suppress the accent hover.
|
||||
const row = container.firstElementChild as HTMLElement | null;
|
||||
const link = container.querySelector("[data-inbox-issue-link]") as HTMLAnchorElement | null;
|
||||
expect(link).not.toBeNull();
|
||||
expect(link?.className).toContain("hover:bg-transparent");
|
||||
expect(link?.className).not.toContain("hover:bg-accent/50");
|
||||
expect(row).not.toBeNull();
|
||||
expect(row?.className).toContain("hover:bg-transparent");
|
||||
expect(row?.className).not.toContain("hover:bg-accent/50");
|
||||
// The overlay link no longer carries the hover wash.
|
||||
expect(link?.className ?? "").not.toContain("hover:bg-transparent");
|
||||
expect(link?.className ?? "").not.toContain("hover:bg-accent/50");
|
||||
|
||||
act(() => {
|
||||
root.unmount();
|
||||
|
|
@ -348,12 +354,17 @@ describe("IssueRow", () => {
|
|||
);
|
||||
});
|
||||
|
||||
// `aria-current="step"` stays on the overlay link (the focusable target),
|
||||
// but the current-step wash moved to the ROOT row band alongside the hover
|
||||
// wash so it paints behind the content.
|
||||
const row = container.firstElementChild as HTMLElement | null;
|
||||
const link = container.querySelector("[data-inbox-issue-link]") as HTMLAnchorElement | null;
|
||||
|
||||
expect(link).not.toBeNull();
|
||||
expect(link?.getAttribute("aria-current")).toBe("step");
|
||||
expect(link?.className).toContain("bg-primary/5");
|
||||
expect(link?.className).not.toContain("border-l-");
|
||||
expect(row?.className).toContain("bg-primary/5");
|
||||
expect(row?.className).not.toContain("border-l-");
|
||||
expect(link?.className ?? "").not.toContain("bg-primary/5");
|
||||
|
||||
act(() => {
|
||||
root.unmount();
|
||||
|
|
@ -446,4 +457,65 @@ describe("IssueRow", () => {
|
|||
root.unmount();
|
||||
});
|
||||
});
|
||||
|
||||
it("renders no bottom divider by default", () => {
|
||||
const root = createRoot(container);
|
||||
|
||||
act(() => {
|
||||
root.render(<IssueRow issue={createIssue()} />);
|
||||
});
|
||||
|
||||
// Dividers are opt-in: without `showDivider` no row-separating border
|
||||
// renders on either the root band or the overlay link.
|
||||
const row = container.firstElementChild as HTMLElement | null;
|
||||
const link = container.querySelector("[data-inbox-issue-link]") as HTMLAnchorElement | null;
|
||||
expect(row).not.toBeNull();
|
||||
expect(row?.className).not.toContain("border-b");
|
||||
expect(link?.className ?? "").not.toContain("border-b");
|
||||
|
||||
act(() => {
|
||||
root.unmount();
|
||||
});
|
||||
});
|
||||
|
||||
it("renders an opt-in bottom divider on the row root when showDivider is set", () => {
|
||||
const root = createRoot(container);
|
||||
|
||||
act(() => {
|
||||
root.render(<IssueRow issue={createIssue()} showDivider />);
|
||||
});
|
||||
|
||||
// The divider lives on the ROOT row band with `last:border-b-0` so the real
|
||||
// last row drops its border — it is not on the overlay link.
|
||||
const row = container.firstElementChild as HTMLElement | null;
|
||||
const link = container.querySelector("[data-inbox-issue-link]") as HTMLAnchorElement | null;
|
||||
expect(row?.className).toContain("border-b");
|
||||
expect(row?.className).toContain("last:border-b-0");
|
||||
expect(link?.className ?? "").not.toContain("border-b");
|
||||
|
||||
act(() => {
|
||||
root.unmount();
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps the hover wash on the row root while the overlay link stays a bare positioning layer", () => {
|
||||
const root = createRoot(container);
|
||||
|
||||
act(() => {
|
||||
root.render(<IssueRow issue={createIssue()} />);
|
||||
});
|
||||
|
||||
const row = container.firstElementChild as HTMLElement | null;
|
||||
const link = container.querySelector("[data-inbox-issue-link]") as HTMLAnchorElement | null;
|
||||
// Hover wash paints behind the content on the root band...
|
||||
expect(row?.className).toContain("hover:bg-accent/50");
|
||||
expect(link?.className ?? "").not.toContain("hover:bg-accent/50");
|
||||
// ...and the overlay link keeps only positioning + focus concerns.
|
||||
expect(link?.className).toContain("absolute");
|
||||
expect(link?.className).toContain("inset-0");
|
||||
|
||||
act(() => {
|
||||
root.unmount();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -57,8 +57,8 @@ interface IssueRowProps {
|
|||
* not crossed out by it.
|
||||
*/
|
||||
chevronInGuide?: boolean;
|
||||
/** Suppress the row divider (parents with expanded children keep visual attachment to their subtree). */
|
||||
hideDivider?: boolean;
|
||||
/** Opt in to a bottom divider on this row (default off; used by views that intentionally keep separators). */
|
||||
showDivider?: boolean;
|
||||
}
|
||||
|
||||
export function IssueRow({
|
||||
|
|
@ -86,7 +86,7 @@ export function IssueRow({
|
|||
onMouseEnter,
|
||||
treeGuides = 0,
|
||||
chevronInGuide = false,
|
||||
hideDivider = false,
|
||||
showDivider = false,
|
||||
}: IssueRowProps) {
|
||||
const issuePathId = issue.identifier ?? issue.id;
|
||||
const identifier = issue.identifier ?? issue.id.slice(0, 8);
|
||||
|
|
@ -170,6 +170,13 @@ export function IssueRow({
|
|||
// when scrubbing the mouse fast across the list.
|
||||
"group relative flex items-start gap-2 rounded-lg py-2.5 pl-2 pr-3 text-sm no-underline text-inherit sm:items-center sm:py-2 sm:pl-1",
|
||||
"[&_button]:relative [&_button]:z-10",
|
||||
// Divider + hover/selected/checklist wash live on the ROOT row band so
|
||||
// the tint paints BEHIND the content and `last:border-b-0` matches the
|
||||
// real last row. Keeping these on the overlay Link (PR #10526) made the
|
||||
// last row keep its border and the hover wash paint over the text.
|
||||
showDivider && "border-b border-border last:border-b-0",
|
||||
selected ? "hover:bg-transparent" : "hover:bg-accent/50",
|
||||
checklistCurrentStep ? "bg-primary/5" : null,
|
||||
className,
|
||||
)}
|
||||
>
|
||||
|
|
@ -183,11 +190,9 @@ export function IssueRow({
|
|||
aria-current={checklistCurrentStep ? "step" : undefined}
|
||||
onClickCapture={() => rememberIssueDetailLocationState(issuePathId, detailState)}
|
||||
className={cn(
|
||||
// Overlay Link keeps ONLY positioning + focus ring so header controls
|
||||
// stay clickable above it; visual washes belong on the root above.
|
||||
"absolute inset-0 rounded-lg no-underline text-inherit focus-visible:z-10 focus-visible:outline-none focus-visible:ring-(length:--rad-3) focus-visible:ring-ring",
|
||||
!hideDivider && "border-b border-border last:border-b-0",
|
||||
selected ? "hover:bg-transparent" : "hover:bg-accent/50",
|
||||
checklistCurrentStep ? "bg-primary/5" : null,
|
||||
className,
|
||||
)}
|
||||
>
|
||||
<span className="sr-only">Open {identifier}: {issue.title}</span>
|
||||
|
|
|
|||
|
|
@ -2097,7 +2097,6 @@ export function IssuesList({
|
|||
onMouseEnter={() => setNavSelectionFromPointer(`issue:${issue.id}`)}
|
||||
treeGuides={depth}
|
||||
chevronInGuide={depth > 0 && hasChildren}
|
||||
hideDivider={hasChildren && isExpanded}
|
||||
checklistStepNumber={checklistStepNumber}
|
||||
checklistCurrentStep={checklistMeta?.currentStepIssueId === issue.id}
|
||||
checklistDependencyChips={checklistDependencyChips}
|
||||
|
|
|
|||
|
|
@ -465,12 +465,14 @@ describe("Inbox toolbar", () => {
|
|||
|
||||
const rows = container.querySelectorAll("[data-inbox-item]");
|
||||
|
||||
const linkOf = (row: Element): HTMLAnchorElement | null =>
|
||||
row.querySelector("a[data-inbox-issue-link]");
|
||||
// The hover wash lives on the IssueRow root band (the overlay link's
|
||||
// parent), not the overlay link itself.
|
||||
const bandOf = (row: Element): HTMLElement | null =>
|
||||
row.querySelector<HTMLAnchorElement>("a[data-inbox-issue-link]")?.parentElement ?? null;
|
||||
|
||||
// Nothing selected before hover — both rows show the hover-accent class.
|
||||
expect(linkOf(rows[0]!)?.className).toContain("hover:bg-accent/50");
|
||||
expect(linkOf(rows[1]!)?.className).toContain("hover:bg-accent/50");
|
||||
expect(bandOf(rows[0]!)?.className).toContain("hover:bg-accent/50");
|
||||
expect(bandOf(rows[1]!)?.className).toContain("hover:bg-accent/50");
|
||||
|
||||
// Hovering paints via CSS `:hover` only — it must NOT flip a row into the
|
||||
// state-selected band (which would swap to hover:bg-transparent). Coupling
|
||||
|
|
@ -482,9 +484,9 @@ describe("Inbox toolbar", () => {
|
|||
rows[1]!.dispatchEvent(new MouseEvent("mouseover", { bubbles: true }));
|
||||
rows[1]!.dispatchEvent(new MouseEvent("mouseenter", { bubbles: false }));
|
||||
});
|
||||
expect(linkOf(rows[0]!)?.className).toContain("hover:bg-accent/50");
|
||||
expect(linkOf(rows[1]!)?.className).toContain("hover:bg-accent/50");
|
||||
expect(linkOf(rows[1]!)?.className).not.toContain("hover:bg-transparent");
|
||||
expect(bandOf(rows[0]!)?.className).toContain("hover:bg-accent/50");
|
||||
expect(bandOf(rows[1]!)?.className).toContain("hover:bg-accent/50");
|
||||
expect(bandOf(rows[1]!)?.className).not.toContain("hover:bg-transparent");
|
||||
|
||||
act(() => {
|
||||
root.unmount();
|
||||
|
|
@ -578,12 +580,13 @@ describe("Inbox toolbar", () => {
|
|||
});
|
||||
const root = createRoot(container);
|
||||
|
||||
const linkOf = (row: Element): HTMLAnchorElement | null =>
|
||||
row.querySelector("a[data-inbox-issue-link]");
|
||||
// The keyboard-selected row swaps to `hover:bg-transparent`; find its index.
|
||||
// The keyboard-selected row swaps to `hover:bg-transparent` on its root
|
||||
// band (the overlay link's parent, where the wash now lives); find its index.
|
||||
const bandOf = (row: Element): HTMLElement | null =>
|
||||
row.querySelector<HTMLAnchorElement>("a[data-inbox-issue-link]")?.parentElement ?? null;
|
||||
const selectedRowIndex = () =>
|
||||
[...container.querySelectorAll("[data-inbox-item]")].findIndex((row) =>
|
||||
linkOf(row)?.className.includes("hover:bg-transparent"),
|
||||
bandOf(row)?.className.includes("hover:bg-transparent"),
|
||||
);
|
||||
|
||||
try {
|
||||
|
|
|
|||
|
|
@ -2689,7 +2689,6 @@ export function Inbox() {
|
|||
issue={issue}
|
||||
issueLinkState={issueLinkState}
|
||||
treeGuides={depth}
|
||||
hideDivider={hasChildren && isExpanded}
|
||||
selected={selected}
|
||||
className={
|
||||
isArchiving
|
||||
|
|
|
|||
Loading…
Reference in New Issue