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
31 changes: 29 additions & 2 deletions DESIGN.md
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ primitive targets):
| Button — hover | default `hover:bg-primary/80`; secondary `color-mix(in oklch, var(--secondary), var(--foreground) 5%)`; destructive soft |
| Input | `h-8 rounded-lg border-input px-2.5 text-base md:text-sm`; dark theme `bg-input/30` |
| Badge | `h-5 px-2 text-xs rounded-4xl`; icons `size-3`. The `quiet` variant alone is `rounded-md` — a square, colourless label for a fact read beside other facts, never a state |
| Menu — surface | `dark` subtree + `bg-popover p-1 rounded-lg ring-1 ring-foreground/10 shadow-md`, `min-w-32`, `duration-100` enter/exit |
| Menu — surface | `dark` subtree + `bg-popover p-1 rounded-lg ring-1 ring-foreground/10 shadow-md`, `min-w-32`, `duration-100` enter and **no exit** — see *Motion* |
| Menu — item | `px-1.5 py-1 text-sm rounded-md focus:bg-accent focus:text-accent-foreground`; destructive item soft |
| Card | `rounded-xl ring-1 ring-foreground/10 text-sm`; `--card-spacing` = `--spacing(4)` (16px; 12px at the `sm` size); footer `bg-muted/50` |
| Dialog — overlay | `bg-black/10 supports-backdrop-filter:backdrop-blur-xs duration-100` |
Expand All @@ -185,7 +185,7 @@ primitive targets):
| Elevation | `ring-1 ring-foreground/10` + a resting shadow — never a coloured border |

`frontend/ui-core/src/primitives/` carries the table itself, not a family resemblance to
it: the heights, the `ring-3` focus treatment, the `duration-100` enter/exit, the
it: the heights, the `ring-3` focus treatment, the `duration-100` enter, the
`--card-spacing` variable and Nova's interaction idioms (uniform disabled opacity, soft
destructive, `/80`-opacity hover, the inverted menu subtree) are the primitives' own
declarations. The table is the contract and the primitives are where it is spelled, so a
Expand Down Expand Up @@ -248,6 +248,33 @@ which is the only place the composition can be checked: Tailwind renders `ring-3
`box-shadow` layer, so "the ring is there" is a question about a computed shadow list
rather than about a class string.

## Motion

Motion orients or confirms, and never stands between somebody and their next action —
*Fast and quiet*, applied. An **enter** animation is free to play: nothing is waiting on
it, because the surface it introduces did not exist a frame ago. An **exit** animation is
not, and the difference is not a matter of taste.

**A floating surface leaves on the frame it is dismissed.** While an exit animation runs,
Radix keeps the content mounted, and the dismissable layer stays mounted with it. A press
on the trigger inside that window is read twice — the trigger toggles the surface open,
and the layer still listening reads the same pointer-down as an interaction outside itself
and dismisses — so the two cancel and the surface never appears. At `duration-100` that
window covers the gap between an `Escape` and the click after it, which makes it a defect
a fast hand meets routinely rather than an edge case. The annotation workspace is where
that tempo is normal, and it is where the behaviour was measured; the fix belongs to the
primitive because every menu in the product shared the flaw.

So `DropdownMenuContent` animates in and not out, and
`frontend/app/e2e/annotate.spec.ts` holds the two presses that would catch a fade being
restored. A surface that genuinely needs an exit — one whose trigger cannot be pressed
again straight away, as a modal's cannot — may keep one; the tooltip keeps its own,
because a hover has no toggle to swallow.

`prefers-reduced-motion` sits above all of this: the base layer in `styles.css` collapses
every animation and transition to a single frame under that query, so none of the above is
something a component opts into.

## Sidebar / Menu

- **`menuColor: inverted`.** Every menu and popover subtree — dropdown content, select
Expand Down
28 changes: 13 additions & 15 deletions frontend/app/e2e/_frame.ts
Original file line number Diff line number Diff line change
Expand Up @@ -294,30 +294,28 @@ export async function expectProgress(page: Page, progress: string): Promise<void
* Radix closes the menu on select, so a scenario pressing two of these opens it
* twice. That is the product's behaviour and not a harness quirk.
*
* The menu is waited for rather than assumed: the trigger *toggles*, so a press
* that arrives while the previous menu is still leaving is read as a dismiss
* instead of an open (see {@link closeOverflow}). Waiting here is what makes that
* fail as "the menu did not open" rather than as a missing item three lines later.
* The menu is waited for rather than assumed, so a menu that does not open fails
* here — as "the menu did not open" — rather than three lines later as a missing
* item.
*/
export async function openOverflow(page: Page): Promise<void> {
await page.getByTestId("more-actions").click();
await expect(page.getByRole("menu")).toBeVisible();
}

/**
* Escape out of the overflow menu, and wait until it has actually left.
* Escape out of the overflow menu, and assert it has actually left.
*
* Nova's menu surface animates out (`data-closed:animate-out` over `duration-100`),
* which means Radix keeps the content — and its dismissable layer — mounted for a
* beat after `open` goes false. A press on the trigger inside that beat hits both
* handlers at once: the trigger toggles the menu open, and the still-listening
* layer reads the same pointer-down as an interaction *outside* itself and
* dismisses. The two cancel, and the menu never appears — which is exactly what
* reopening the overflow straight after an `Escape` used to run into.
* The wait is the assertion that `Escape` closed the menu at all, and it is on the
* menu leaving the DOM rather than on a clock — per
* `tests/scripts/e2e_discipline.test.mjs`.
*
* So the close is a state to wait on, not a keystroke to fire and forget. The wait
* is on the menu leaving the DOM, which is also the assertion that `Escape` closed
* it at all — no clock anywhere, per `tests/scripts/e2e_discipline.test.mjs`.
* It is not a workaround for a surface that leaves slowly. The menu has no exit
* animation, deliberately, so there is no window in which it is still mounted and
* swallowing the next press — `DESIGN.md`'s *Motion* argues why, and
* `annotate.spec.ts` presses the trigger straight after an `Escape` to keep it
* true. A scenario that wants to reopen the menu immediately should do exactly
* that rather than reach for this.
*/
export async function closeOverflow(page: Page): Promise<void> {
await page.keyboard.press("Escape");
Expand Down
35 changes: 35 additions & 0 deletions frontend/app/e2e/annotate.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3517,3 +3517,38 @@ test("a press on the suggest panel never reaches the picture underneath", async
await expect(page.getByTestId("suggest-adjustments")).toBeVisible();
expect(asks(sent)).toBe(before);
});

/**
* ## Reopening a menu that was just dismissed
*
* `DESIGN.md`'s *Motion*: a floating surface leaves on the frame it is dismissed,
* because an animated exit keeps Radix's dismissable layer mounted and the next
* press on the trigger is then read as both an open and an outside-dismiss, which
* cancel. These two scenarios are the presses that catch a fade being restored —
* both failed against `duration-100` on the way out, the first in four runs of
* five and the second in five.
*
* Neither uses `closeOverflow`. Its wait is what a suite does to stay honest about
* a surface that is still leaving; the product's contract is that there is nothing
* to wait for.
*/
test("the overflow reopens on a press straight after Escape", async ({ page }) => {
const sent: Request[] = [];
await openJob(page, sent);

await openOverflow(page);
await page.keyboard.press("Escape");
await page.getByTestId("more-actions").click();
await expect(page.getByRole("menu")).toBeVisible();
});

test("the overflow reopens on a press straight after a second Escape", async ({ page }) => {
const sent: Request[] = [];
await openJob(page, sent);

await openOverflow(page);
await page.keyboard.press("Escape");
await page.keyboard.press("Escape");
await page.getByTestId("more-actions").click();
await expect(page.getByRole("menu")).toBeVisible();
});
6 changes: 3 additions & 3 deletions frontend/ui-core/src/annotator/AddClassDialog.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@
* where every other refusal here renders.
*/

import { Plus, X } from "lucide-react";
import { IconPlus, IconX } from "@tabler/icons-react";
import { useEffect, useRef, useState, type JSX } from "react";

import { asApiError } from "../data/errors";
Expand Down Expand Up @@ -634,7 +634,7 @@ export function AddClassDialog({
onBank?.(banked);
}}
>
<X className="size-3" aria-hidden="true" />
<IconX className="size-3" aria-hidden="true" />
</button>
</li>
))}
Expand Down Expand Up @@ -830,7 +830,7 @@ export function AddClassDialog({
disabled={pending || !readyForm}
onClick={addAnother}
>
<Plus className="size-4" aria-hidden="true" />
<IconPlus className="size-4" aria-hidden="true" />
Create and add another
<kbd className="ml-1 rounded border border-border px-1 font-mono text-xs text-muted-foreground">
⌘↵
Expand Down
Loading
Loading