diff --git a/docs/menus.md b/docs/menus.md index 1b0af48af..4193be54f 100644 --- a/docs/menus.md +++ b/docs/menus.md @@ -86,16 +86,32 @@ and a column of icons there is decoration competing with the values you actually ## Surface details -- The hover fill is **inset by the same amount on every side and rounded** — a chip inside the - menu, not a band across it. The inset is taken _out of_ the row, never added to it: horizontally - padding gives up what margin takes so labels don't move, and vertically the fill is shorter by - the same amount so the row pitch is unchanged. Insetting by growing the row is how the menu ends - up taller than it started. -- The fill's height only holds because the label's `lineHeight` is pinned. Leave it to the platform - and content outgrows `minHeight`, which then does nothing. -- **The item's inset is the menu's only spacing.** Rows sit apart because each holds itself in, and - a separator gets clearance from its neighbours for free. Don't give the separator a margin of its - own — that is two numbers controlling one gap, and they will disagree. +- The hover fill is **inset from the surface's edges and rounded** — a chip inside the menu, not a + band across it. The inset is taken _out of_ the row, never added to it: padding gives up what + margin takes, so labels sit at the same 13pt they always did. Insetting by growing the row is how + the menu ends up taller than it started. +- **A row is as tall as what is driving it** — 28pt for a pointer, 40pt below `md` for a thumb. The + split is on breakpoint, not on `presentation`: the compact popover that `compactMode` defaults to + is worked with a thumb just as a sheet is, and sizing off the sheet would leave it at the desktop + height. `md` is where `useIsCompactFormFactor` divides, so row height and the popover/sheet choice + turn over together. +- The desktop height only holds because the label's `lineHeight` is pinned — 18 line + 8 padding + + 2 border is exactly 28. Leave it to the platform and content outgrows `minHeight`, which then does + nothing. Compact is the other way round: `minHeight` leads and the label centres in it. +- **A row owns its fill; `MenuPage` owns the spacing between rows.** The vertical inset above the + first row and below the last, and the gap between rows, are the page's — one knob each, + `MENU_ROW_GAP` being the one a redesign turns. A row that carried vertical margin would be + serving both at once, and since margins don't collapse it would land as one unit at the edges and + two between rows, so shrinking the gap would eat the inset with it. Horizontal inset does stay on + the row: there is one left edge and one right edge, so nothing is doubling up. +- `MenuPage` is also what provides a page's depth, so the popover and the sheet cannot disagree + about what a page is. Every entry point goes through it — root, flyout, pushed sheet page. +- The gap reaches a page's **direct children**. A group of rows wrapped in a `View` of its own drops + out of it; at the current gap of zero there is nothing to lose, but anything above zero wants that + wrapper to carry the page style too. +- The separator is the one row-level thing that asks for vertical space of its own, and now that + rows carry none, that is still one number controlling one gap. It takes no horizontal margin and + the page has no horizontal padding, so the rule runs the full width of the surface. - Separators use `borderAccent`, the same colour the surface outlines itself with. `border` sits between `surface1` and `surface2`, which put it within a hair of the hover fill and made separators disappear against a hovered row. diff --git a/packages/app/src/components/ui/menu/menu-item.tsx b/packages/app/src/components/ui/menu/menu-item.tsx index ffa828b36..562d2fdfa 100644 --- a/packages/app/src/components/ui/menu/menu-item.tsx +++ b/packages/app/src/components/ui/menu/menu-item.tsx @@ -17,7 +17,7 @@ import { Check, CheckCircle } from "lucide-react-native"; import { LoadingSpinner } from "@/components/ui/loading-spinner"; import { Tooltip, TooltipContent, TooltipTrigger } from "@/components/ui/tooltip"; import type { Theme } from "@/styles/theme"; -import { useMenuContext } from "./menu-context"; +import { MenuDepthProvider, useMenuContext } from "./menu-context"; const ThemedCheck = withUnistyles(Check); const ThemedCheckCircle = withUnistyles(CheckCircle); @@ -28,20 +28,55 @@ const mutedMapping = (theme: Theme) => ({ color: theme.colors.foregroundMuted }) const successMapping = (theme: Theme) => ({ color: theme.colors.palette.green[500] }); /** - * Height of the filled part of a row, which together with the uniform inset puts the row pitch - * back at the 36pt it was before the fill was inset at all — the space is redistributed, not - * added. + * Height of the filled part of a row, which is also its hit target. * - * It only holds if the label's line box is pinned: 18 line + 8 padding + 2 border is exactly - * this. Leave the text's `lineHeight` to the platform and the content outgrows `minHeight`, - * which then does nothing and the rows drift taller again. + * A pointer aims; a thumb lands, so the row is sized by what is driving it. The split is on + * breakpoint rather than on `presentation`, because the compact popover — what `compactMode` + * defaults to — is worked with a thumb just as a sheet is, and would keep the desktop height if + * the sheet were the thing being asked about. `md` is where `useIsCompactFormFactor` divides, so + * this and the popover/sheet choice always turn over together. + * + * On desktop the number is exactly the content: 18 line + 8 padding + 2 border. Leave the text's + * `lineHeight` to the platform and the content outgrows `minHeight`, which then does nothing and + * the rows drift taller again. On compact, `minHeight` leads instead and the label centres in it. */ -const MENU_ITEM_HEIGHT = 28; +const MENU_ITEM_HEIGHT = { xs: 40, md: 28 } as const; const MENU_ITEM_LINE_HEIGHT = 18; +/** + * Space between two rows, owned by the page rather than the rows. Zero because only one row is + * ever filled at a time, so a gap here buys nothing on the common frame and only costs pitch. + * + * This is the knob a redesign turns. Note it applies to a page's direct children, so a group of + * rows wrapped in a `View` of its own would not receive it — at zero there is nothing to lose, + * but anything above it wants the wrapper to carry the same style. + */ +const MENU_ROW_GAP = 0; + /** Action status for menu items with loading/success feedback. */ export type ActionStatus = "idle" | "pending" | "success"; +/** + * One page of rows — the root surface, a flyout, or a pushed sheet page. + * + * It owns the vertical spacing of the menu: the inset above the first row and below the last, + * and the gap between rows. A row knows how to draw its own fill and nothing about where it sits + * in a list, so a redesign retunes the rhythm here and never touches `MenuItem`. + * + * Horizontal inset stays on the row. There is one left edge and one right edge, so a row's + * horizontal margin is not standing in for anything else, and leaving it there keeps labels, + * hints and the custom headers callers render into a surface aligned on the same 12pt. + * + * It also carries the page's depth, so the two presentations cannot disagree about what a page is. + */ +export function MenuPage({ depth, children }: PropsWithChildren<{ depth: number }>): ReactElement { + return ( + + {children} + + ); +} + export function MenuLabel({ children, style, @@ -247,6 +282,10 @@ export function MenuItem({ } const styles = StyleSheet.create((theme) => ({ + page: { + paddingVertical: theme.spacing[1], + gap: MENU_ROW_GAP, + }, labelContainer: { paddingHorizontal: theme.spacing[3], paddingTop: theme.spacing[2], @@ -260,10 +299,15 @@ const styles = StyleSheet.create((theme) => ({ // made separators vanish against a hovered row. `borderAccent` is the colour the menu surface // already outlines itself with, so the divider reads as part of the same frame. // - // No margin of its own: the items above and below already hold themselves off it. Giving the - // rule its own spacing on top of theirs is how you end up tuning two numbers to control one gap. + // The one thing on a page that wants more room than the row gap gives it, so it says so here. + // That is one number controlling one gap: rows no longer carry vertical spacing of their own, + // so there is nothing left for this to double up with. + // + // No horizontal margin, and the page has no horizontal padding, so the rule still runs the + // full width of the surface rather than reading as an inset tick between two chips. separator: { height: 1, + marginVertical: theme.spacing[1], backgroundColor: theme.colors.borderAccent, }, hintContainer: { @@ -278,21 +322,22 @@ const styles = StyleSheet.create((theme) => ({ fontSize: theme.fontSize.sm, color: theme.colors.foreground, }, - // The fill is inset by the same amount on every side and rounded, so a hovered row reads as a - // chip sitting inside the menu. That inset is the only thing that spaces this menu: rows sit - // apart because each holds itself in, and a separator gets clearance from its neighbours for - // free instead of carrying its own margin. + // The fill is inset from the surface's edges and rounded, so a hovered row reads as a chip + // sitting inside the menu rather than a band across it. // - // The inset is taken out of the row, not added to it. Horizontally that means padding gives up - // what margin takes — margin 4 + padding 8 + border 1 lands the label at the same 13pt it - // always sat at. Vertically the fill itself is shorter by the same 8, so the pitch is unchanged - // and the menu did not grow; it just stopped filling every row edge to edge. + // The inset is taken out of the row, not added to it: margin 4 + padding 8 + border 1 lands the + // label at the same 13pt it always sat at. + // + // Horizontal only. Vertical spacing — the inset above the first row and below the last, and the + // gap between rows — belongs to `MenuPage`. A margin here would have to be both of those at + // once, and since margins do not collapse it would land as one unit at the edges and two + // between rows, so shrinking the gap would eat the inset with it. item: { flexDirection: "row", alignItems: "center", minHeight: MENU_ITEM_HEIGHT, gap: theme.spacing[2], - margin: theme.spacing[1], + marginHorizontal: theme.spacing[1], paddingHorizontal: theme.spacing[2], paddingVertical: theme.spacing[1], borderWidth: theme.borderWidth[1], diff --git a/packages/app/src/components/ui/menu/menu-surface.tsx b/packages/app/src/components/ui/menu/menu-surface.tsx index 74d439c3e..0daa7f1f2 100644 --- a/packages/app/src/components/ui/menu/menu-surface.tsx +++ b/packages/app/src/components/ui/menu/menu-surface.tsx @@ -20,7 +20,8 @@ import { type ContextBridge, } from "@/components/ui/isolated-bottom-sheet-modal"; import type { Theme } from "@/styles/theme"; -import { useMenuContext, MenuContextProvider, MenuDepthProvider } from "./menu-context"; +import { useMenuContext, MenuContextProvider } from "./menu-context"; +import { MenuPage } from "./menu-item"; import { currentPageId, isSubPageOpen } from "./menu-navigation"; import { AnchoredSurface, MenuOverlay } from "./menu-overlay"; import type { Alignment, Placement } from "./menu-anchor"; @@ -224,7 +225,7 @@ function MenuPopoverSurface({ scrollable={scrollable} testID={testID} > - {children} + {children} {openPages.map(({ page, depth }) => ( - {page.content} + {page.content} ); } @@ -363,12 +364,12 @@ function MenuSheetSurface({ {openPage ? ( <> - {openPage.content} + {openPage.content} ) : ( <> {sheetTitle ? : null} - {children} + {children} )}