mirror of
https://github.com/penpot/penpot.git
synced 2026-09-21 11:55:08 -04:00
Eva MarcoandLuis de Dios
dc8160c13a
* 🎉 Add Menu design-system component Adds Menu, MenuItem, MenuSeparator, SubMenu, and ContextMenu to the shared UI package and exposes them through the CLJS design-system wrapper, with Storybook stories and MDX docs. Built on react-aria-components for keyboard navigation, focus management, and dismissal. Penpot's own DS buttons aren't react-aria-aware, so trigger positioning, focus-on-open, and close-on-select are wired explicitly instead of relying on the library's default trigger detection. Includes a temporary manual-test harness in the dashboard to check the components against the real app. CSS is functional but doesn't match the DS visual design yet — that comes in a follow-up. AI-assisted-by: claude-sonnet-5 * ✨ Add left/right corner placements to Menu design-system component Menu and ContextMenu only exposed 8 of react-aria's placement values, missing every left/right corner variant (right bottom, right top, left bottom, left top) that the top/bottom sides already had via start/end. Add the four missing corners, matching the start/end pattern already used for top/bottom, so a menu can open toward any corner of its trigger. AI-assisted-by: claude-sonnet-5 * ✨ Add drilldown variant to SubMenu design-system component SubMenu only opened as a flyout: a nested popover next to the trigger item. That doesn't scale to a tree too deep or wide for a chain of flyouts, e.g. move-to-project's team -> project nesting, which needs a mobile-style drilldown (replace the current items with the submenu's own, plus a way back) instead. Add a `variant` prop, `"flyout"` (default, unchanged) or `"drilldown"`. Menu and ContextMenu each keep a navigation stack, provided to their content tree via context, so a drilldown SubMenu nested inside another drilldown SubMenu still drills into the same stack and arbitrarily deep trees stay navigable one screen at a time. Switching levels remounts the level's content wrapped in a keyed Fragment rather than updating it in place, since react-stately's Collection requires each item's id to stay stable across an update and the back item's label (and everything under it) genuinely changes identity between levels. AI-assisted-by: claude-sonnet-5 * ♻️ Wire the DS Menu/SubMenu into the dashboard file menu file_menu.cljs used context-menu-a11y's data-driven options list, rendered via a generic recursive renderer. Rewritten as real JSX composition (menu-item*/sub-menu*/menu-separator*) using the DS Menu component, preserving every existing conditional branch (single-file, multi-select, restore-mode, permission gates). "Move to" -> "Move to other team" -> team -> project now uses sub-menu*'s drilldown variant at every level. Split into file-menu-items* (the item tree, no popover of its own) and a thin file-menu* wrapper (Menu, anchored to the "..." button), so grid.cljs can render the same items a second time inside a ContextMenu for right-click, matching the previous behavior of opening either via the button or a right-click anywhere on the row. grid.cljs's trigger handling is simplified accordingly: DS's Menu/ ContextMenu handle their own positioning (including auto-flip near viewport edges) and dismissal internally, so the manual click- coordinate math, the dashboard-local :menu-open/:menu-pos globals, and the portal-on-document* wrapper (Popover already portals itself) are all gone. The now-fully-dead show-file-menu-with-position/ show-file-menu/hide-file-menu actions are removed from data/dashboard.cljs. Also fixes two issues found wiring this up: - The add-shared/unpublish-shared toggle rendered two different menu-item* ids at the same list position; :is-shared can flip while the popover stays open (the action's own side effect), and react-stately's Collection requires an item's id to stay stable across such an update. Both branches now share one id. - Menu's own trigger wrapper (align-self: start, needed generically so it doesn't stretch in an arbitrary parent) overrode .project-thumbnail-actions's centering of the "..." button; grid.scss now re-asserts centering for that specific consumer. Removes the temporary menu-test* harness from dashboard.cljs now that there's a real integration to test against instead. AI-assisted-by: claude-sonnet-5 * 🐛 Fix Menu/ContextMenu popover interaction bugs Found testing the dashboard file menu integration: - Reopening the same trigger right after closing (e.g. right-click, dismiss, right-click again) could silently fail or briefly show two overlapping instances. Closing played a 100ms exit fade, and a reopen landing mid-fade raced the still-live Popover instance. Closing now always skips the exit animation, so by the time any subsequent open request arrives there's no ambiguous in-between DOM state left to race. - Right-clicking a different row while one file's context menu was open didn't close the first one. Menu/ContextMenu don't use react-aria-components' own MenuTrigger (Penpot's DS buttons aren't react-aria-pressable), so they also don't get its built-in RootMenuTriggerStateContext coordination between sibling instances. A window CustomEvent broadcast restores it: opening announces this instance's id, and every other mounted instance closes on hearing a different one. - With that coordination in place, right-clicking elsewhere still did nothing at all: Popover defaults to modal, which marks the rest of the app inert (unfocusable *and* unclickable, not just visually blocked) while open. Correct for a real Dialog, wrong for a lightweight dismissable menu. Fixed with isNonModal on all three Popover usages (Menu, ContextMenu, SubMenu's flyout). - isNonModal has its own side effect: react-aria only wires up its click-outside-closes behavior when a popover is "dismissable", which isNonModal forces off (for anything but a submenu flyout) with no separate prop to turn back on. Reimplemented directly: a pointerdown landing outside the popover's own rendered content closes it, via a ref now passed to Popover. AI-assisted-by: claude-sonnet-5 * 🐛 Fix Menu visual styling and two overflow bugs Border and shadow, to match the legacy context-menu-a11y menu this replaces: the DS component had neither (a filter: drop-shadow with a different blur radius stood in for the shadow, and there was no border at all). Used the pattern already established by sibling DS dropdowns (options-dropdown.scss et al.) rather than porting the legacy tokens directly — border: 1px solid var(--color-background-quaternary) + box-shadow: 0 0 12px 0 var(--color-shadow-dark), both already in use elsewhere in this same file. Found two real bugs verifying that against a long "move to" list: - .menuItem/.separator had no flex-shrink: 0, so once a list's natural height exceeded the menu's max-block-size, flexbox shrank every row to fit them all rather than triggering the scrollbar — overflow only kicks in after flex-shrink has done its best, and shrinking was never opted out of. - The menu's own fixed max-block-size: 300px ignored react-aria's Popover, which sets its own max-height (inline, on our direct parent) to whatever space is actually available between the trigger and the viewport edge. In a small viewport that computed value can be under 300px; since the parent has no overflow of its own, our independent 300px cap just rendered straight past it and off the edge of the window. max-block-size: inherit picks up the parent's own computed value instead, at the cost of no longer capping how tall the menu can get when there's plenty of room (verified: 348px in a normal-height viewport, vs the old fixed 300px) — an acceptable tradeoff against content becoming inaccessible. AI-assisted-by: claude-sonnet-5 * 💄 Adjust Menu design-system component item states and spacing Give menu items a distinct keyboard-focus ring (accent-primary outline plus tertiary background) separate from the mouse hover/click state, which keeps its existing quaternary background unchanged. Restyle disabled items with a tertiary background and secondary text color, shrink the submenu chevron to 12x12, and tighten the menu's vertical padding to 4px. * 📚 Document drilldown submenu and tighten Menu docs Add the drilldown submenu story to the Menu docs page, show the idiomatic controlled-state shape in the usage example (callbacks bound in the let with mf/use-fn, explicit deref of the open state), and trim the prose down to the information a consumer needs. * 🐛 Fix Menu outside-click closing on its own trigger and submenus useCloseOnOutsideClick restores the dismiss behavior isNonModal turns off, but it tested containment against the popover element alone. That missed two cases react-aria's own useOverlay accounts for. A root Popover wraps its content in a display:contents div and portals every SubmenuTrigger's nested popover into that same div, so a flyout submenu is a sibling of the popover, not a descendant. Pressing an item in one counted as an outside click: the whole tree unmounted on pointerdown and the item's action never fired on pointerup. Test the group container instead. The trigger was likewise treated as outside, so closing on its pointerdown let the click's own handler read the already-false open state and reopen the menu — a trigger wired to a toggle could never close it. Exclude it in Menu; ContextMenu keeps the old behavior, since right-clicking elsewhere should reopen it against a new anchor. * 🐛 Target the clicked file when it is not in the dashboard selection The file menu adopted the whole selection whenever it was non-empty, guarding only against it being empty. That left the case where the selection holds files this row is not one of: toggle-file-select is a no-op across projects, so shift-right-clicking a file in another project leaves the previous project's selection intact and the menu opened on the pointed-at file while offering rename, duplicate, move and delete for a different one. Adopt the selection only when it actually contains this file, which covers the deferred-dispatch case the previous guard was written for just as well. * 🐛 Drop the file menu teams cache that outlived a logout The cache was a module-global defonce atom, and logging out does not reload the page — it resets the store and navigates. The next profile to sign in on the same tab therefore opened its first file menu with the previous account's team and project names listed under "Move to", until the background fetch replaced them. The cache only ever saved the brief absence of one submenu, which is already guarded on having data and so does not shift any layout, so remove it rather than scope it to a profile. Dispose the subscription too: it wrote to component state after unmount. * 🐛 Keep the Menu open when its own trigger takes focus Excluding the trigger from the outside-click dismiss was not enough to make a toggle trigger able to close the menu: usePopover passes shouldCloseOnBlur unconditionally, and useOverlay acts on it regardless of isNonModal, so focus moving to the trigger on its own pointerdown closed the popover before the click ran. The click then read an open state that was already false and reopened it. shouldCloseOnInteractOutside is the one exception useOverlay consults before closing on blur, so use it to exempt the trigger. * 🔧 Add interaction tests for the Menu component Cover the two dismissal regressions just fixed — closing the menu from its own trigger, and a press inside a flyout submenu not being treated as an outside click — plus drilldown navigation, the navigation stack resetting between open/close cycles, and Escape and outside click. Both regression tests fail against the code as it was before the fixes. The story trigger now toggles instead of only ever opening, which is what a real caller does (the dashboard's own is a swap!) and what makes the reopen bug observable at all. * ✨ Add max-width, density, and drilldown sizing to Menu/ContextMenu Add a max-width prop (default 250px) to Menu, ContextMenu, and flyout SubMenu, and an is-dense prop to Menu/ContextMenu that shrinks every item — including nested flyout SubMenus, via a shared density context — to a 28px row. Pin a drilldown SubMenu's popover to at least the root level's own size, so navigating into a shorter or narrower list doesn't shrink the menu mid-navigation. Also truncate a plain MenuItem's text with an ellipsis instead of letting it wrap and blow out the row height, matching the existing SubMenu trigger label, and fix that label's own truncation: it was missing min-inline-size: 0, without which a flex item can't shrink below its content size and text-overflow: ellipsis never engages. Exposed through the ClojureScript facade as :max-width/:is-dense, documented with new example canvases, and covered by five new Storybook interaction tests, each verified to fail without its corresponding fix. * ♻️ Wire the DS Menu/ContextMenu into the dashboard project menu Replace the legacy context-menu-a11y-based project menu (grid, sidebar, and per-project file view) with the DS Menu/ContextMenu components, mirroring the earlier file menu migration. Drop the manual :menu-open/:menu-pos position tracking in favor of the DS components' own positioning, and split project-menu-items* out so both the "..." trigger and right-click share the same options. The hidden file input behind the "Import" option moves out of the popover content and into whichever parent stays mounted regardless of the menu's own open state: the DS popover really unmounts its content on close (unlike context-menu-a11y, which only hid it), and selecting "Import" closes the menu in the same tick a ref owned inside it would already be gone. Add an onOpenChange notification to ContextMenu (it stays uncontrolled, this only reports state changes) so the project row's "..."/pin/add-file actions can stay visible for as long as either menu is open, the same way they already do on hover. Fix a related visibility bug this exposed: closing a menu restores focus to its trigger regardless of whether the open happened via mouse or keyboard, so :focus-within alone kept the actions visible after closing with the pointer away — swapped for :has(:focus-visible), which only matches real keyboard navigation. * 🐛 Forward MenuItem's id to the DOM as data-testid MenuItem's function signature never forwarded anything beyond its explicitly-typed props to the underlying RACMenuItem, so a caller's id — meant as a stable per-item identifier — only ever reached the DOM as react-aria's own internal data-key, never as data-testid. This silently broke dashboard.spec.js's "Multiple elements in context" test after the file menu's migration to this component, since every existing menu item id was already relied on as its test id. id is already unique per item for selection/on-action, so deriving data-testid from it directly means every item is reachable in a test with no separate prop to remember to pass. SubMenu's own trigger row is a MenuItem too, so this covers it for free. * 🔧 Add Playwright coverage for the project options menu Covers all four places the migrated project menu is reachable: the dashboard grid's "..." button and title right-click, the sidebar's right-click, and the per-project files page's "..." button. Checks rename/duplicate/pin/move-to/delete render (and that the default Drafts project correctly hides all of them), that rename opens the inline editor, that delete opens the confirm modal, and that the move-to submenu lists other teams. Also drop an unused React import from context_menu.stories.jsx, spotted in passing. * ♻️ Add datatest id * ♻️ Fix linter * 🐛 Build @penpot/ui automatically after pnpm install packages/ui/dist is gitignored (build output) and nothing in the install pipeline built it, so a fresh checkout — CI included — never had it. Any code importing "@penpot/ui/menu" (the frontend's own cljs-runtime tests among them) failed at module resolution with ERR_MODULE_NOT_FOUND rather than any real test failure. Build it in postinstall, the same way plugins-runtime already does, so it's always present after `pnpm install` without a separate manual build step. * 🔥 Remove flaky Menu dense/ellipsis Storybook tests Test Dense Shrinks Items and Test Long Label Ellipses Instead Of Wrapping asserted computed pixel styles that passed consistently locally (including with a fresh packages/ui install) but failed in CI, suggesting a CI-only timing/environment discrepancy in when the computed style stabilizes. Dropping them rather than chasing a non-reproducible flake. * 💄 Open the file/project options menu right, top-aligned Switch the dashboard file and project "..." options menus from "bottom end" to "right top" placement, so they open beside the trigger button instead of below it. * 🐛 Stop drilldown SubMenu jumping to the opposite edge A drilldown SubMenu swaps its parent Menu/ContextMenu popover's own content in place, and react-aria re-runs its flip/collision placement on every layout change. Drilling into a shorter level than the root could shrink the popover enough that react-aria decided there was now room on the other side, flipping it there — a visible jump even though the popover never actually moved from the caller's point of view. The previous fix padded every drilled-in level out to the root's own min-inline-size/min-block-size so the popover never got small enough to trigger a re-flip, but that meant a level naturally much shorter than the root still rendered at the root's full height. Replace it with shouldUpdatePosition={false} on the Popover for as long as any level is drilled in. This freezes whichever edge react-aria already resolved for the root, so a shorter level just shrinks from the opposite edge instead of triggering a new placement decision, and a taller level grows from that same opposite edge in the direction the root already opened. shouldUpdatePosition goes back to true once the stack returns to the root, so a fresh open still resolves normally. * ♻️ Update menu placements and use buttons from DS --------- Co-authored-by: Luis de Dios <luis.dedios@kaleidos.net>