feat(app): size menu rows for the input driving them
Rows are 40pt below md and 28pt above it. The split is on breakpoint rather than presentation because the compact popover, which compactMode defaults to, is worked with a thumb just as a sheet is. Move vertical spacing off the row onto a new MenuPage, which also provides a page's depth so the popover and the sheet cannot disagree about what a page is. A row carrying vertical margin would serve both the edge inset and the inter-row gap at once, and margins do not collapse.
This commit is contained in:
+26
-10
@@ -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.
|
||||
|
||||
@@ -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 (
|
||||
<MenuDepthProvider value={depth}>
|
||||
<View style={styles.page}>{children}</View>
|
||||
</MenuDepthProvider>
|
||||
);
|
||||
}
|
||||
|
||||
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],
|
||||
|
||||
@@ -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}
|
||||
>
|
||||
<MenuDepthProvider value={0}>{children}</MenuDepthProvider>
|
||||
<MenuPage depth={0}>{children}</MenuPage>
|
||||
</AnchoredSurface>
|
||||
{openPages.map(({ page, depth }) => (
|
||||
<MenuFlyout
|
||||
@@ -291,7 +292,7 @@ function MenuFlyout({
|
||||
onPointerLeave={handleHoverOut}
|
||||
testID={testID}
|
||||
>
|
||||
<MenuDepthProvider value={depth + 1}>{page.content}</MenuDepthProvider>
|
||||
<MenuPage depth={depth + 1}>{page.content}</MenuPage>
|
||||
</AnchoredSurface>
|
||||
);
|
||||
}
|
||||
@@ -363,12 +364,12 @@ function MenuSheetSurface({
|
||||
{openPage ? (
|
||||
<>
|
||||
<MenuSheetHeader title={openPage.title} onBack={menu.goBack} />
|
||||
<MenuDepthProvider value={depth}>{openPage.content}</MenuDepthProvider>
|
||||
<MenuPage depth={depth}>{openPage.content}</MenuPage>
|
||||
</>
|
||||
) : (
|
||||
<>
|
||||
{sheetTitle ? <MenuSheetHeader title={sheetTitle} onBack={null} /> : null}
|
||||
<MenuDepthProvider value={0}>{children}</MenuDepthProvider>
|
||||
<MenuPage depth={0}>{children}</MenuPage>
|
||||
</>
|
||||
)}
|
||||
</BottomSheetScrollView>
|
||||
|
||||
Reference in New Issue
Block a user