feat(theme): tokenize the popup height caps - #430
Open
IgorShevchik wants to merge 2 commits into
Open
Conversation
The combobox/select content slot capped at a literal 40rem in three theme files, and menus at 40vh in two more. Neither was overridable without patching the theme, and — the reason this is worth doing rather than tidy — neither was distinguishable from an upstream literal during a port. Upstream nuxt/ui caps the same slot at 15rem; taking that hunk wholesale shortens every list by more than half, and the result looks entirely plausible in review. Two tokens, not one: --max-height-popup-list: 40rem Select, SelectMenu, InputMenu --max-height-popup-menu: 40vh DropdownMenu, ContextMenu A result list is bounded in absolute units because its readable length does not depend on the window; a menu is bounded against the viewport so it cannot dominate the screen. Different units, tuned independently — lengthening an autocomplete has no business resizing context menus. test/utils/popup-height-tokens.spec.ts fails if a cap goes back to a literal, and asserts each theme still has a cap to check, so the guard cannot go quiet by the slot moving. 350 snapshots regenerated. Every changed line is the cap class and nothing else — verified against the diff rather than assumed. Refs #73 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
Review found the sweep incomplete and the guard weaker than its own docblock claimed. editor-suggestion-menu.ts still capped at a literal 40vh, and the editor's slash, mention and emoji menus all extend it. Its docblock points at dropdown-menu.ts — it is the same kind of popup — so "override this and every menu changes" was false for three of them. Now capped through --max-height-popup-menu. The guard now sweeps src/theme/*-menu.ts rather than trusting its own list, since the literal it missed was in a menu nobody had listed. Scoped to the menu family on purpose: modal.ts, page-aside.ts and the prose themes also cap in viewport units and should, because a dialog sized against the viewport is not a popup sharing this ceiling. Two more holes closed: - the token values were never asserted. Moving them out of the theme files took them out of ~350 snapshots that had held 40rem verbatim, so editing sizes.css to upstream's 15rem passed the entire suite — the exact revert this spec exists to prevent; - the cap check accepted either token in any theme, so capping a result list against the viewport token passed. It is a map now: each theme is pinned to the token that belongs to it. Docs: the "shrinks rather than overflowing" claim was wrong for Select with content.position="item-aligned", where the popper reports no available height and only the ceiling applies. Corrected, and the editor menus added to the table. The skill's design-system guidelines mirror that docs page and had not grown the section, so an agent reading the skill would not have learned the tokens exist. Added, and skills/index.json regenerated. Refs #73 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012MsMuj8Fic9tjWVjyEyrxc
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue
Resolves #73
Type of change
Description
The combobox/select content slot capped at a literal
40remin three theme files, and menus at40vhin two more. Not overridable without patching the theme — and, the reason this is worth doing rather than merely tidy, not distinguishable from an upstream literal during a port. Upstream nuxt/ui caps the same slot at15rem; taking that hunk wholesale shortens every list by more than half, and the result looks entirely plausible in review. Nothing renders wrong; the list is just short.Two tokens, not one. #73 suggested a single
--b24ui-popper-max-heightand asked whether the menus'40vhshould join it. It should not. A result list is bounded in absolute units because its readable length does not depend on the window; a menu is bounded against the viewport so it cannot dominate the screen. The units differ and the two are tuned independently — lengthening an autocomplete has no business resizing context menus.Both stay ceilings rather than fixed heights: the cap is still the smaller of the token and the space the popper actually has, so a popup near the viewport edge shrinks and scrolls instead of overflowing.
The tokens sit in
air-design-tokens/tw-style/sizes.cssbeside the--max-height-*scale, with a note on why they are not part of it:40remfalls betweenxl(36rem) and2xl(42rem) because it was tuned against real Bitrix24 lists rather than picked off a ramp.Guard
test/utils/popup-height-tokens.spec.tsfails if a cap goes back to a literal — which is exactly what the next port will try to do. It also asserts each theme still has a cap to check, so the guard cannot go quiet by the slot moving, and that the themes reference the tokens the CSS declares, so a rename cannot go one-sided (CSS resolves a missing variable silently).Snapshots
350 regenerated. The diff is 350 insertions and 350 deletions, and every changed line contains the cap class and nothing else — checked against the diff, not assumed. Verified before regenerating that the only difference in a failing snapshot was that one class.
Docs
New "Popup height" section on the design-system page with both defaults, an override example, and the reason they are separate. Carries the
Soonbadge per the repo convention — the docs site redeploys on merge, the tokens ship on the next npm release.Porting note
Recorded as a §2 invariant in .sync/PORTING.md, including the instruction to keep the two tokens separate, since merging them is the obvious-looking simplification.
Verification
pnpm run typecheckeslinton all five themes and the new specGenerated by Claude Code