mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-05 03:51:54 +01:00
Polish Done-reviewing handoff dialog UX (PAP-10575)
Address non-blocking UX-review nits on the Done-reviewing handoff:
- a11y: the disabled "Done reviewing" rail CTA used a non-focusable
<span> as the tooltip trigger, so keyboard/SR users couldn't learn
why it was disabled. Make the Button itself the trigger via
aria-disabled (kept in tab order) instead of the `disabled` attribute,
guard onClick, and keep the dimmed visual via aria-disabled: classes.
- copy: replace the agent-jargon "a real wake" dialog subhead with plain
English ("We'll notify them so they pick the review back up").
- copy: add a clarifying title to the "Just close" ghost button
("Close this dialog without notifying the owner").
Update DocumentReviewRail tests to assert the aria-disabled/inert
behavior and that the handler fires when review can be finished.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
1 parent
a4a0d63bf8
commit
d60106d09a
3 files changed
+56
-16
No files matched your search
@@ -246,7 +246,8 @@ describe("DocumentReviewRail", () => {
|
||||
expect(container.textContent).toContain("Orphaned feedback (1)");
|
||||
});
|
||||
|
||||
it("disables Done reviewing for read-only viewers", async () => {
|
||||
it("keeps Done reviewing keyboard-focusable but inert for read-only viewers", async () => {
|
||||
const onDoneReviewing = vi.fn();
|
||||
await act(() =>
|
||||
root.render(
|
||||
<DocumentReviewRail
|
||||
@@ -254,11 +255,39 @@ describe("DocumentReviewRail", () => {
|
||||
canReview={false}
|
||||
canFinishReview={false}
|
||||
latestRevisionId="rev-1"
|
||||
doneReviewingDisabledReason="You don't have edit access"
|
||||
onDoneReviewing={onDoneReviewing}
|
||||
{...baseHandlers}
|
||||
/>,
|
||||
),
|
||||
);
|
||||
const done = container.querySelector<HTMLButtonElement>('[data-testid="rail-done-reviewing"]');
|
||||
expect(done?.disabled).toBe(true);
|
||||
// aria-disabled (not the `disabled` attribute) so it stays in the tab order and can
|
||||
// surface the tooltip explaining *why* it's disabled to keyboard / screen-reader users.
|
||||
expect(done?.getAttribute("aria-disabled")).toBe("true");
|
||||
expect(done?.hasAttribute("disabled")).toBe(false);
|
||||
// ...but clicking it must not fire the handoff.
|
||||
await act(() => done!.click());
|
||||
expect(onDoneReviewing).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("fires the Done reviewing handler when the viewer can finish review", async () => {
|
||||
const onDoneReviewing = vi.fn();
|
||||
await act(() =>
|
||||
root.render(
|
||||
<DocumentReviewRail
|
||||
reviewIndex={makeIndex()}
|
||||
canReview
|
||||
canFinishReview
|
||||
latestRevisionId="rev-1"
|
||||
onDoneReviewing={onDoneReviewing}
|
||||
{...baseHandlers}
|
||||
/>,
|
||||
),
|
||||
);
|
||||
const done = container.querySelector<HTMLButtonElement>('[data-testid="rail-done-reviewing"]');
|
||||
expect(done?.getAttribute("aria-disabled")).toBeNull();
|
||||
await act(() => done!.click());
|
||||
expect(onDoneReviewing).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
});
|
||||
@@ -388,19 +388,29 @@ function ReviewRailBody({
|
||||
<div className="border-t border-border p-2">
|
||||
<Tooltip>
|
||||
<TooltipTrigger asChild>
|
||||
<span className="block">
|
||||
<Button
|
||||
type="button"
|
||||
className="w-full"
|
||||
size="sm"
|
||||
disabled={!canFinishReview}
|
||||
onClick={onDoneReviewing}
|
||||
data-testid="rail-done-reviewing"
|
||||
>
|
||||
<CheckCircle2 className="mr-1.5 h-4 w-4" aria-hidden="true" />
|
||||
Done reviewing
|
||||
</Button>
|
||||
</span>
|
||||
{/*
|
||||
Use aria-disabled (not the `disabled` attribute) so the button stays
|
||||
in the tab order and remains the tooltip trigger: keyboard and screen-reader
|
||||
users can focus it to learn *why* it's disabled, instead of hitting a dead
|
||||
control. onClick is guarded so the action can't fire while disabled.
|
||||
*/}
|
||||
<Button
|
||||
type="button"
|
||||
className="w-full aria-disabled:cursor-not-allowed aria-disabled:opacity-50"
|
||||
size="sm"
|
||||
aria-disabled={!canFinishReview || undefined}
|
||||
onClick={(event) => {
|
||||
if (!canFinishReview) {
|
||||
event.preventDefault();
|
||||
return;
|
||||
}
|
||||
onDoneReviewing?.();
|
||||
}}
|
||||
data-testid="rail-done-reviewing"
|
||||
>
|
||||
<CheckCircle2 className="mr-1.5 h-4 w-4" aria-hidden="true" />
|
||||
Done reviewing
|
||||
</Button>
|
||||
</TooltipTrigger>
|
||||
{!canFinishReview ? (
|
||||
<TooltipContent>
|
||||
|
||||
@@ -93,7 +93,7 @@ export function DoneReviewingDialog({
|
||||
<DialogHeader>
|
||||
<DialogTitle>Done reviewing</DialogTitle>
|
||||
<DialogDescription>
|
||||
Hand the document back to its owner with an optional note and a real wake.
|
||||
Hand the document back to its owner with an optional note. We'll notify them so they pick the review back up.
|
||||
</DialogDescription>
|
||||
</DialogHeader>
|
||||
|
||||
@@ -171,6 +171,7 @@ export function DoneReviewingDialog({
|
||||
variant="ghost"
|
||||
onClick={() => onOpenChange(false)}
|
||||
disabled={submitting}
|
||||
title="Close this dialog without notifying the owner"
|
||||
data-testid="done-reviewing-just-close"
|
||||
>
|
||||
Just close
|
||||
|
||||
Reference in new issue
Block a user