feat(layout-library): header thumbnail quick-switch for layouts#1941
Merged
Conversation
Adds a LayoutQuickSwitch control to the header so switching layouts is a one-click, thumbnail-led action instead of a modal round-trip. Production data shows most users keep 1–2 layouts (82% ≤2), so recognition-by-shape in the header fits how layouts are actually used. - Trigger shows the active layout's thumbnail + a chevron; the dropdown lists every layout (preview + name, ✓ on active) and switches on click - "New layout" and "Manage layouts" live in the dropdown; Manage opens the existing layout manager (now reached via the switch, not a standalone button) - Inline layout-name rename in the header is preserved unchanged - Drops the now-orphaned header.layouts / header.openLayoutManager strings The layout manager modal keeps its full management UI; switching from it still works as a secondary path. i18n added across all 9 locales.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Greptile SummaryReplaces the old "Layouts" text button in the header with
Confidence Score: 5/5Safe to merge — functional logic, error handling, and module boundaries are all correct. The core switching logic delegates errors to useLayoutSwitcher which already shows toasts, so no silent failures. The barrel export and import wiring are clean. The only open items are accessibility nits (aria-current semantics, unlabelled menu role, focus-on-Escape) that don't affect runtime behaviour. The three accessibility comments are all in LayoutQuickSwitch.tsx; no other files need special attention. Important Files Changed
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
src/features/layout-library/components/LayoutQuickSwitch/LayoutQuickSwitch.tsx:148-163
**`aria-current` is non-standard for menu selection state**
`aria-current="true"` on a `role="menuitem"` signals "the current item in a navigation set" (breadcrumbs, tabs) — not "the selected option in a choice group". Screen readers vary in how they announce this; many will say "current" rather than "selected" or "checked". The ARIA-compliant pattern for a single-select menu is `role="menuitemradio"` with `aria-checked="true"` on the active entry and `aria-checked="false"` on all others. Wrapping the list in a `role="group"` with an `aria-label` rounds this out.
### Issue 2 of 3
src/features/layout-library/components/LayoutQuickSwitch/LayoutQuickSwitch.tsx:140-143
**`role="menu"` has no accessible name**
The WAI-ARIA authoring practices require elements with `role="menu"` to have an accessible name so screen readers can announce what the menu is for. Currently it renders as just "menu" with no context. Consider adding `aria-label={t('header.manageLayouts')}` or a more descriptive label so the purpose of the popup is clear.
### Issue 3 of 3
src/features/layout-library/components/LayoutQuickSwitch/LayoutQuickSwitch.tsx:102-104
**Escape key doesn't restore focus to the trigger**
When a keyboard or screen-reader user opens the dropdown (via Space/Enter on the trigger), navigates into the list, and presses Escape to dismiss, focus is left stranded on whichever menu item had it rather than returned to the trigger button. Per the codebase's focus-management guideline ("restore focus after modal/dialog closes"), a `triggerRef` should be attached to the trigger button and called on Escape: `triggerRef.current?.focus()`.
Reviews (3): Last reviewed commit: "fix(layout-library): quick-switch trigge..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds a header-level LayoutQuickSwitch control to switch layouts via an always-visible thumbnail trigger, replacing the previous “Layouts” button and moving “Manage layouts” into the dropdown (opening the existing layout manager modal). This fits the layout-library UX by making layout switching a single interaction while preserving the existing inline layout-name rename behavior.
Changes:
- Replaces the header “Layouts” button with a thumbnail-led
LayoutQuickSwitchdropdown trigger. - Adds
LayoutQuickSwitchcomponent + unit tests; updatesHeadertests to mock the new control. - Updates i18n across locales (removes old header keys; adds new switch/manage strings) and allowlists the new component for design-system checks.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shell/Header/Header.tsx | Swaps the old layout-manager button for LayoutQuickSwitch and routes “Manage” to the existing modal. |
| src/shell/Header/Header.test.tsx | Mocks LayoutQuickSwitch to keep header tests focused while still testing modal open/close wiring. |
| src/features/layout-library/components/LayoutQuickSwitch/LayoutQuickSwitch.tsx | New dropdown component for thumbnail-led layout switching + manage/new actions. |
| src/features/layout-library/components/LayoutQuickSwitch/LayoutQuickSwitch.test.tsx | Unit tests covering open/switch/active-noop/new/manage paths. |
| src/features/layout-library/components/LayoutQuickSwitch/index.ts | Barrel export for the new component. |
| src/i18n/locales/en.ts | Replaces old header strings with header.switchLayout / header.manageLayouts. |
| src/i18n/locales/en.json | Replaces old header strings with header.switchLayout / header.manageLayouts. |
| src/i18n/locales/de.json | Removes orphaned header keys; adds new switch/manage keys. |
| src/i18n/locales/es.json | Removes orphaned header keys; adds new switch/manage keys. |
| src/i18n/locales/fr.json | Removes orphaned header keys; adds new switch/manage keys. |
| src/i18n/locales/ja.json | Removes orphaned header keys; adds new switch/manage keys. |
| src/i18n/locales/nb.json | Removes orphaned header keys; adds new switch/manage keys. |
| src/i18n/locales/nl.json | Removes orphaned header keys; adds new switch/manage keys. |
| src/i18n/locales/pt-BR.json | Removes orphaned header keys; adds new switch/manage keys. |
| src/i18n/locales/sv.json | Removes orphaned header keys; adds new switch/manage keys. |
| src/i18n/locales/uk.json | Removes orphaned header keys; adds new switch/manage keys. |
| scripts/design-system-allowlist.txt | Allowlists LayoutQuickSwitch.tsx for design-system checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+20
to
+26
| const { activeLayoutId, library, switchLayout, createNewLayout } = useLayoutSwitcher(); | ||
| const [open, setOpen] = useState(false); | ||
| const containerRef = useRef<HTMLDivElement>(null); | ||
|
|
||
| const hasLayouts = library.entries.length > 0; | ||
| const activeEntry = library.entries.find((e) => e.id === activeLayoutId) ?? library.entries[0]; | ||
|
|
Comment on lines
+1
to
+5
| import { useCallback, useEffect, useRef, useState } from 'react'; | ||
| import { useLayoutSwitcher } from '@/shared/hooks'; | ||
| import { LayoutThumbnail } from '@/shell/LayoutThumbnail'; | ||
| import { layoutId } from '@/core/types'; | ||
| import { useTranslation } from '@/i18n'; |
Comment on lines
+66
to
+71
| aria-label={t('header.switchLayout', { name: activeEntry.name })} | ||
| > | ||
| <span className="flex h-6 w-8 items-center justify-center overflow-hidden rounded border border-stroke-subtle bg-surface"> | ||
| <LayoutThumbnail | ||
| preview={activeEntry.preview} | ||
| size={30} |
- export LayoutQuickSwitch from the feature barrel and import it there (Greptile); verified the barrel's static re-export of the lazily-loaded LayoutManagerModal does not change the main bundle - mark the active layout with aria-current; the check icon is now decorative (aria-hidden) so screen readers announce the current item once - extract Icon / Thumb / MenuAction helpers, collapsing ~50 lines of duplicated inline SVG and shared menu-item markup
Owner
Author
|
Thanks! Addressed in 4abfcd6:
Plus a couple of self-review improvements in the same commit:
Re-verified: typecheck ✅ · lint ✅ · 33 tests ✅ · Playwright visual check (dropdown renders identically post-refactor). |
In shared-layout-preview / embed mode the active layout (SHARED_PREVIEW_ID) is not a library entry, so the previous `find() ?? entries[0]` fallback mislabeled the header trigger with the first library layout's name and thumbnail (Copilot). The trigger now derives its name + preview from the live layout store (computePreview) when there's no matching library entry, so it always reflects what's on screen. The dropdown's switch list is unchanged. Added a test for the shared-preview path.
Owner
Author
|
Good catch — fixed in ea219fe. Shared-preview trigger: in shared-layout-preview / embed mode the active layout is (The three comments were facets of this one issue — the imports concern is resolved since the fix adds Verified: typecheck ✅ · lint ✅ · 34 tests ✅ · module-boundaries ✅ · knip ✅ |
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
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.
What & why
Switching layouts was a modal round-trip. This adds a
LayoutQuickSwitchto the header so it's a one-click, thumbnail-led action. Production data shows most users keep 1–2 layouts (82% ≤2), so recognition-by-shape in the header fits how layouts are actually used — and you flagged early that the thumbnail must stay.This is stage 3 of 3:
Tags foundation✅ feat(bin-designer): add synced organization tags to saved designs #1936Designs-manager rebuild✅ feat(bin-designer): organize saved designs with tags, filtering, and bulk actions #1939Changes
header.layouts/header.openLayoutManagerstrings.Scope notes
Verification
LayoutQuickSwitch(6 tests — render/open/switch/active-noop/new/manage) + updatedHeadertests (27).check:i18n(keys/interpolation/values/unused) green.