Skip to content

Commit adfd4e1

Browse files
bloveclaude
andcommitted
fix(react): menus size to their labels, and a disabled item looks disabled
Every popover took `popoverStyle`'s fixed 240px width. That is the dialog width — `FilterMenu`'s operator select over its value input, whose controls stretch to their container — and it made the tool panel's pin menu ("Pin left" / "Pin right" / "Unpin" / "Auto width") a mostly empty 240px box anchored at a 20px kebab, spilling past the grid and over the page beside it. The header's ⋮ ("Group by this column") and `AddGroupMenu` had the same box. Split the module's one export in two over a shared `placement()`: `popoverStyle` keeps the fixed column for the dialog and the cell editors, and `menuPopoverStyle` sizes a menu to its content between a 160px floor and the dialog's 240px. The horizontal clamp stays bound to 240 for both — a content-sized menu can only be narrower, so the right edge is safe with no measure-then-reposition pass. Two things the pin menu's own semantics needed and the stylesheet never gave it. `[data-pretable-menu-item]` had no `:disabled` rule, so the placement the column is ALREADY in — the menu's way of saying where the column is — kept the enabled color, a pointer cursor and the `:hover` highlight, reading as the one item to click. It now takes the tool pane's standard disabled treatment and the hover rule carries `:not(:disabled)`. And a `role= "separator"` now divides the one-shot placement commands from the auto-width mode bit, which stays open when toggled; `useMenuKeyboard` roves over `[data-pretable-menu-item]` only, so it is not a focus stop. Verified in a browser on a production build, not just in jsdom: the pin menu measures 160px (was 240), the disabled item computes `--pretable-text-dim` with `cursor: default` and stays transparent under a real pointer hover while an enabled sibling still highlights, the separator paints a 1px rule, the filter dialog is untouched at 240px, and Pin right / Pin left still move the column between the pinned sections. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent d7e3bf6 commit adfd4e1

9 files changed

Lines changed: 175 additions & 11 deletions

File tree

.changeset/menu-popover-sizing.md

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
---
2+
"@pretable/react": patch
3+
"@pretable/ui": patch
4+
---
5+
6+
The grid's list-shaped menus — the tool panel's column kebab (pin placement +
7+
auto width), the `+ Add group` menu and the header's `` — now size to their
8+
own labels, dim the item that is already the current state, and rule off the
9+
mode bit from the commands.
10+
11+
All three shared `popoverStyle`, which stamps a fixed 240px width: the right
12+
call for `FilterMenu`, a dialog whose form controls stretch to their container,
13+
and wrong for a menu of four short labels, which was drawn as a mostly empty
14+
rectangle spilling well past the grid. Menus now take `menuPopoverStyle`
15+
content width between a 160px floor and the dialog's 240px, still clamped
16+
horizontally against 240 so the right edge stays safe without measuring.
17+
18+
The pin menu disables the placement the column is already in, but
19+
`[data-pretable-menu-item]` had no disabled treatment at all: the disabled item
20+
kept the enabled color, the pointer cursor, and the hover highlight, so the one
21+
item that cannot be chosen read as the obvious one to click. It now takes the
22+
tool pane's standard disabled treatment (`--pretable-text-dim`, default
23+
cursor), and the hover rule skips it.
24+
25+
`ColumnRowMenu` gained a `role="separator"` between the one-shot pin commands
26+
(which close the menu) and the auto-width checkbox (which stays open) —
27+
`[data-pretable-menu-separator]`, styled by `grid.css`, and not a focus stop.

packages/react/src/__tests__/popover-position.test.ts

Lines changed: 50 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
import { afterEach, describe, expect, it } from "vitest";
22

3-
import { popoverStyle } from "../overlay/popover-position";
3+
import {
4+
menuPopoverStyle,
5+
popoverStyle,
6+
} from "../overlay/popover-position";
47

58
const originalWidth = window.innerWidth;
69
const originalHeight = window.innerHeight;
@@ -102,3 +105,49 @@ describe("popoverStyle", () => {
102105
expect(popoverStyle(rect(100, 200, 120, 300)).maxHeight).toBeUndefined();
103106
});
104107
});
108+
109+
describe("menuPopoverStyle", () => {
110+
it("sizes to its content instead of the dialog's fixed column", () => {
111+
setViewport(1024, 768);
112+
const style = menuPopoverStyle(rect(100, 200, 120, 300));
113+
114+
// The defect this exists for: a four-item pin menu drawn 240px wide.
115+
expect(style.width).toBe("max-content");
116+
expect(style.maxWidth).toBe(240);
117+
expect(popoverStyle(rect(100, 200, 120, 300)).width).toBe(240);
118+
});
119+
120+
it("keeps a floor, so a one-word menu is still menu-shaped", () => {
121+
setViewport(1024, 768);
122+
expect(menuPopoverStyle(rect(100, 200, 120, 300)).minWidth).toBe(160);
123+
});
124+
125+
it("places itself exactly as a dialog does", () => {
126+
setViewport(1024, 768);
127+
const anchor = rect(100, 200, 120, 300);
128+
const { width, minWidth, maxWidth, ...placement } =
129+
menuPopoverStyle(anchor);
130+
const { width: dialogWidth, ...dialogPlacement } = popoverStyle(anchor);
131+
132+
expect(placement).toEqual(dialogPlacement);
133+
expect(dialogWidth).toBe(240);
134+
expect(width).toBe("max-content");
135+
expect(minWidth).toBe(160);
136+
expect(maxWidth).toBe(240);
137+
});
138+
139+
it("clamps against the widest it could be, never past the right edge", () => {
140+
setViewport(400, 768);
141+
// Same clamp as the dialog: a content-sized menu can only be narrower,
142+
// so the bound holds without measuring the rendered menu.
143+
expect(menuPopoverStyle(rect(100, 380, 120, 400)).left).toBe(152);
144+
});
145+
146+
it("flips upward when there is no room below", () => {
147+
setViewport(1024, 768);
148+
const style = menuPopoverStyle(rect(720, 200, 740, 300));
149+
150+
expect(style.top).toBeUndefined();
151+
expect(style.bottom).toBe(768 - 720 + 4);
152+
});
153+
});

packages/react/src/__tests__/tool-panel.test.tsx

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -820,6 +820,27 @@ describe("columns section pin menu", () => {
820820
]);
821821
});
822822

823+
it("rules off the one-shot commands from the mode-bit checkbox", () => {
824+
const h = mountColumnsSection();
825+
openKebab(h.kebabFor("Bravo")!);
826+
827+
const menu = document.querySelector("[data-pretable-column-menu]")!;
828+
const separators = menu.querySelectorAll("[data-pretable-menu-separator]");
829+
expect(separators).toHaveLength(1);
830+
expect(separators[0]).toHaveAttribute("role", "separator");
831+
832+
// Between the last command and the checkbox, not anywhere in the list:
833+
// the two kinds of item are what it divides.
834+
const children = Array.from(menu.children);
835+
const items = h.menuItems();
836+
expect(children.indexOf(separators[0]!)).toBe(children.indexOf(items[2]!) + 1);
837+
expect(children.indexOf(items[3]!)).toBe(children.indexOf(separators[0]!) + 1);
838+
839+
// A separator is not an item: the roving contract queries menu items, so
840+
// it must not become a focus stop.
841+
expect(separators[0]).not.toHaveAttribute("data-pretable-menu-item");
842+
});
843+
823844
it("disables the matching pin item for an already-pinned column", () => {
824845
const h = mountColumnsSection();
825846
openKebab(h.kebabFor("Alpha")!);

packages/react/src/overlay/popover-position.ts

Lines changed: 43 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,15 @@
11
import type { CSSProperties } from "react";
22

3+
/**
4+
* The DIALOG width — one column of form controls (`FilterMenu`'s operator
5+
* select over its value input) and the cell editors' panels. It doubles as
6+
* the horizontal-clamp bound for every popover, menus included: clamping
7+
* against the widest a popover can be is what makes the right edge safe
8+
* without measuring anything.
9+
*/
310
const WIDTH = 240;
11+
/** A list-shaped menu narrower than this reads as a stray chip, not a menu. */
12+
const MENU_MIN_WIDTH = 160;
413
/** Gap between the anchor and the popover. */
514
const GAP = 4;
615
/** Minimum breathing room kept against every viewport edge. */
@@ -9,7 +18,7 @@ const MARGIN = 8;
918
const MIN_SPACE = 160;
1019

1120
/**
12-
* Fixed-position style from the anchor rect.
21+
* Where the popover sits: `position: fixed` coordinates from the anchor rect.
1322
*
1423
* Horizontally the popover is *clamped* into the viewport (never flipped).
1524
* Vertically it opens below the anchor, and flips above it when there is not
@@ -18,10 +27,14 @@ const MIN_SPACE = 160;
1827
* never has to be measured. No `max-height` is set — each popover's CSS owns
1928
* its own height cap.
2029
*/
21-
export function popoverStyle(rect: DOMRect): CSSProperties {
30+
function placement(rect: DOMRect): CSSProperties {
2231
const vw = typeof window !== "undefined" ? window.innerWidth : 1024;
2332
const vh = typeof window !== "undefined" ? window.innerHeight : 768;
2433

34+
// Clamped against WIDTH even for a content-sized menu, which can only be
35+
// narrower: the popover is then further from the right edge than it needed
36+
// to be, never past it. Measuring the real width would mean a layout pass
37+
// and a second paint at a corrected position.
2538
const left = Math.min(rect.left, vw - WIDTH - MARGIN);
2639

2740
const spaceBelow = vh - rect.bottom - GAP - MARGIN;
@@ -32,7 +45,34 @@ export function popoverStyle(rect: DOMRect): CSSProperties {
3245
position: "fixed",
3346
...(flip ? { bottom: vh - rect.top + GAP } : { top: rect.bottom + GAP }),
3447
left: Math.max(MARGIN, left),
35-
width: WIDTH,
3648
zIndex: 50,
3749
};
3850
}
51+
52+
/**
53+
* A DIALOG-shaped popover: a fixed {@link WIDTH} column, because the form
54+
* controls inside it stretch to their container and a shrink-wrapped one
55+
* would be as narrow as its widest option string.
56+
*/
57+
export function popoverStyle(rect: DOMRect): CSSProperties {
58+
return { ...placement(rect), width: WIDTH };
59+
}
60+
61+
/**
62+
* A MENU-shaped popover: sized to its own longest label instead of the
63+
* dialog's column. `Pin right` and `Group by this column` are ~60px and
64+
* ~150px of text; both were drawn in a 240px box, which left an item's click
65+
* target and its words in different halves of a mostly empty rectangle.
66+
*
67+
* A floor and a cap rather than free-running content width — the labels are
68+
* caller data (a column header, for `AddGroupMenu`), so neither end can be
69+
* left to them.
70+
*/
71+
export function menuPopoverStyle(rect: DOMRect): CSSProperties {
72+
return {
73+
...placement(rect),
74+
width: "max-content",
75+
minWidth: MENU_MIN_WIDTH,
76+
maxWidth: WIDTH,
77+
};
78+
}

packages/react/src/pretable-surface.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ import { MenuButton } from "./column-menu/MenuButton";
166166
import { FilterMenu, FunnelButton } from "./filter-menu";
167167
import { resolveColumnOptions } from "./filter-menu/filter-operators";
168168
import { OverlayPortal } from "./overlay/OverlayPortal";
169-
import { popoverStyle } from "./overlay/popover-position";
169+
import { menuPopoverStyle, popoverStyle } from "./overlay/popover-position";
170170
import { useHeaderPopover } from "./overlay/useHeaderPopover";
171171
import { useHydrated } from "./use-hydrated";
172172
import {
@@ -7843,7 +7843,7 @@ export function PretableSurface<
78437843
columnId={menuOpenState.columnId}
78447844
grouped={snapshot.rowGroups.includes(menuOpenState.columnId)}
78457845
label={col.header ?? menuOpenState.columnId}
7846-
style={popoverStyle(menuOpenState.rect)}
7846+
style={menuPopoverStyle(menuOpenState.rect)}
78477847
onClose={closePopover}
78487848
onSelect={selectColumnMenuAction}
78497849
/>

packages/react/src/tool-panel/ColumnRowMenu.tsx

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,12 @@ export function ColumnRowMenu({
9393
{messages.toolPanelPinLabel({ pinned: item.pinned })}
9494
</button>
9595
))}
96+
{/* Divides the one-shot placement COMMANDS above from the mode bit
97+
below — two kinds of item with two activation behaviors, which
98+
without a rule between them read as one flat list of four. A real
99+
role="separator": `useMenuKeyboard` roves over
100+
[data-pretable-menu-item] only, so it is skipped by construction. */}
101+
<div role="separator" data-pretable-menu-separator="" />
96102
{/* "Let the grid manage this column's width" — a mode bit over the
97103
auto-width store, NOT a fit-to-content action (spec B1/Fact 2).
98104
The check glyph trails the label so the label's position is

packages/react/src/tool-panel/ColumnsSection.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ import { GROUP_COLUMN_ID } from "@pretable/core";
1414
import { ROW_SELECT_COLUMN_ID } from "../constants";
1515
import { CheckIcon, GripIcon, OverflowIcon } from "../icons";
1616
import type { AutoWidthSetReader } from "../pretable-model";
17-
import { popoverStyle } from "../overlay/popover-position";
17+
import { menuPopoverStyle } from "../overlay/popover-position";
1818
import { useHeaderPopover } from "../overlay/useHeaderPopover";
1919
import { ColumnRowMenu } from "./ColumnRowMenu";
2020
import type { ToolPanelColumnsMessages } from "./messages";
@@ -559,7 +559,7 @@ export function ColumnsSection({
559559
label={open.label}
560560
pinned={open.entry.pinned ?? null}
561561
messages={messages}
562-
style={popoverStyle(menu.rect)}
562+
style={menuPopoverStyle(menu.rect)}
563563
onClose={closeMenu}
564564
// The menu stays open (its comment carries the checkbox-vs-
565565
// command rationale), so no pending-focus arming here: the row

packages/react/src/tool-panel/grouping/GroupingSection.tsx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {
1010
import type { ColumnType } from "@pretable/core";
1111

1212
import { CloseIcon, GripIcon } from "../../icons";
13-
import { popoverStyle } from "../../overlay/popover-position";
13+
import { menuPopoverStyle } from "../../overlay/popover-position";
1414
import { useHeaderPopover } from "../../overlay/useHeaderPopover";
1515
import type { GroupingSectionMessages } from "../messages";
1616
import type { ToolDropTarget, ToolRowRect } from "../tool-panel-drop-target";
@@ -459,7 +459,7 @@ export function GroupingSection({
459459
<AddGroupMenu
460460
messages={messages}
461461
options={ungrouped}
462-
style={popoverStyle(menu.rect)}
462+
style={menuPopoverStyle(menu.rect)}
463463
onClose={closeAddMenu}
464464
onSelect={(columnId) => {
465465
applyRowGroups([...groupedIds, columnId]);

packages/ui/grid.css

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1111,9 +1111,19 @@
11111111
text-align: left;
11121112
cursor: pointer;
11131113
}
1114-
:where([data-pretable-menu-item]:hover) {
1114+
:where([data-pretable-menu-item]:hover:not(:disabled)) {
11151115
background: var(--pretable-selection-bg);
11161116
}
1117+
/* The standard disabled treatment — dims by token and drops the pointer
1118+
affordance, exactly as the tool pane's action buttons do. Load-bearing
1119+
here rather than cosmetic: a pin menu disables the placement the column
1120+
is ALREADY in, so with the enabled items' color, the hover highlight and
1121+
a pointer cursor, the item that says what is true reads as the one thing
1122+
to click. */
1123+
:where([data-pretable-menu-item]:disabled) {
1124+
color: var(--pretable-text-dim);
1125+
cursor: default;
1126+
}
11171127
:where([data-pretable-menu-item]:focus-visible) {
11181128
outline: 2px solid var(--pretable-focus-ring);
11191129
outline-offset: -2px;
@@ -1129,6 +1139,17 @@
11291139
justify-content: space-between;
11301140
gap: 8px;
11311141
}
1142+
/* Divides KINDS of item, not groups of them: above it the one-shot
1143+
commands that close the menu, below it the mode bits that stay open.
1144+
A border on a role="separator" element — the menu's own accessible
1145+
divider — rather than a border-top on the item after it, so the rule
1146+
survives that item moving or a second one joining it. */
1147+
:where([data-pretable-menu-separator]) {
1148+
height: 0;
1149+
margin: 4px 0;
1150+
border: 0;
1151+
border-top: 1px solid var(--pretable-rule);
1152+
}
11321153

11331154
/* Enum combobox listbox (cell editor) */
11341155
:where([data-pretable-enum-editor]) {

0 commit comments

Comments
 (0)