Theme navbar improvements and theme card follow-ups - #5908
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 644f5b7299
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…, and raise mode toggle above navbar
209b013 to
45a557f
Compare
… toggle into it, and fix narrow-container layout
45a557f to
3b5996e
Compare
backspace
left a comment
backspace
left a comment
There was a problem hiding this comment.
[Claude Code 🤖] This review went after the parts of the change that reach outside the theme cards — the shared BoxelDropdown edit, the base-realm exports, and the new NavBar's measurement/reactivity loop — plus the section-list plumbing that now has three places to stay in sync. It did not evaluate the visual design, and it did not audit the brand-guide JSON content edits beyond confirming that dropping the persisted cssImports is inert (cssImports is a computed field, so those values were already ignored).
One blocking issue: the BoxelDropdown focus change drops the trigger refocus for every dropdown in the app, not just menus that scroll. Everything else below is non-blocking.
The Codex bot's P1 about deferring the initial overflow measurement does not apply at this head: trackFit installs the ResizeObserver and nothing else, leaning on its first async delivery exactly as that comment asks. Safe to dismiss.
Recommendations, in order:
- Name the trigger as focus-trap's return-focus node instead of skipping the focus move entirely — thread on
closeSkippingFocusinpackages/boxel-ui/src/components/dropdown/index.gts. Blocking. - Delete
ThemeVisualizer's now-unreachable@toggleDarkMode— thread on its signature intheme-dashboard.gts. - Pin nav-ids-equal-body-ids in a test so
style-reference.gts's neweqchain can't fail open — thread on the{{#each this.visibleSections}}. - Cover the menu → scroll path, untested in both the overflow and compact menus — thread on
scrollFromMenu. - Fix the
--dsr-nav-heightcomment — thread on.dsr-nav'sheight.
One question outside the threads: packages/experiments-realm/Theme/ loses five theme instances (bubblegum, doom64, neo-brutalism, retro-arcade, starry-night). They still adopt from a live Theme in card-api, nothing in the repo references them, and neither the description nor the commit that removes them mentions the deletion. Intentional?
Red check: Percy reports one visual change awaiting review — expected for this change, but it needs a human approval before that check goes green. Nothing else is red.
Adjacent, out of scope: with SimpleNavBar gone, ThemeDashboard's <:navBar> block has no in-repo consumer left. Reasonable to keep as an extension point for card authors, but nothing exercises it now.
| // BasicDropdown's plain close() refocuses the trigger with a bare focus(), | ||
| // whose scroll-into-view cancels a smooth scroll a menu action just started; | ||
| // the focus trap already returns focus on deactivate, with preventScroll | ||
| @action closeSkippingFocus(dropdown: Dropdown) { | ||
| dropdown.actions.close(undefined, true); | ||
| } |
There was a problem hiding this comment.
[Claude Code 🤖] skipFocus drops the trigger refocus entirely, and focus-trap's return focus is not an equivalent substitute: returnFocusOnDeactivate targets state.nodeFocusedBeforeActivation — whatever document.activeElement was when the trap activated — and setReturnFocus is unset. That equals the trigger only when the user clicked the trigger to open it.
NewFileButton (packages/host/app/components/operator-mode/new-file-button.gts) is opened from operatorModeStateService.state.newFileDropdownOpen through @initiallyOpened, so the trap activates at mount with focus still on <body>; closing it now leaves focus on <body> rather than returning it to the New File trigger. Same for any registerAPI-driven open, and for browsers that don't focus a <button> on click.
The scroll comes from the bare focus(), not from focusing the trigger — so keep the target and drop the scroll by naming the trigger as the return-focus node, where the preventScroll you added already applies:
@action triggerReturnFocus(dropdown: Dropdown, previouslyFocused: HTMLElement) {
return (
document.querySelector(
`[aria-controls='ember-basic-dropdown-content-${dropdown.uniqueId}']`,
) ?? previouslyFocused
);
}A plain selector string works for setReturnFocus too, but focus-trap's getNodeForOption throws when it resolves to nothing, and the overlay trigger in operator-mode-overlays.gts unmounts with its {{#if isHovered}} — hence the function form with the fallback.
Regression introduced here, in a component every dropdown in the app shares. Blocking.
|
|
||
| export class ThemeVisualizer extends GlimmerComponent<{ | ||
| Args: { | ||
| toggleDarkMode?: () => void; |
There was a problem hiding this comment.
[Claude Code 🤖] @toggleDarkMode no longer has a producer — structured-theme.gts, style-reference.gts and detailed-style-reference.gts each swapped it for id='preview' in this change, so this arg and the {{#if @toggleDarkMode}}<ModeToggle …> branch in the visualizer header are unreachable. Worth removing along with the move: left in place, the next card that renders a visualizer wires up a second toggle competing with the navbar's over the same isDarkMode.
Dead machinery this change creates, so the fix belongs here. Non-blocking.
| <section id='wallpapers' class='wallpapers'> | ||
| <h2>Wallpaper Gallery</h2> | ||
| <@fields.wallpaperImages /> | ||
| {{#each this.visibleSections as |navSection|}} |
There was a problem hiding this comment.
[Claude Code 🤖] This chain fails open: an id in visibleSections with no matching eq branch renders nothing while the nav still lists it, and the resulting link is silent — scrollToSectionFrom returns without a word when querySelector misses the id. It is also the second hand-maintained enumeration of the same list (contentSections + GUIDE_SECTIONS is the first), so the two drift the first time a section is added to either.
Every id is covered today, so this is not a live bug. The cheap guard is in the test rather than the template: for a themed and a theme-less edit view, assert the [data-test-style-ref-section] ids equal the nav's [data-test-theme-nav-item] ids (dropping preview). The new theme-less case pins body order but never checks the two lists against each other, which is exactly the invariant this chain can break.
Non-blocking; follow-up-shaped but small enough to land here.
| // the dropdown content is portaled outside the card, so the menu action | ||
| // cannot walk up from its own element; scroll from the captured nav instead | ||
| private scrollFromMenu = (sectionId: string) => { | ||
| if (this.navElement) { | ||
| scrollToSectionFrom(this.navElement, sectionId); | ||
| } | ||
| }; |
There was a problem hiding this comment.
[Claude Code 🤖] Nothing exercises this. The overflow test opens the "more" menu and asserts the item is listed, then stops; the compact branch (data-test-theme-nav-menu, allMenuItems) never renders in any test. So the one non-obvious mechanism in the navbar — menu content is portaled out of the card, so the section lookup has to start from the captured navElement rather than from the clicked element — has no coverage, and it degrades silently: if registerNav ever stops running, scrollFromMenu no-ops with no error.
Ask: add a case below the compact threshold that opens the hamburger and asserts a menu item click resolves to its section, rather than only that the item is listed. Non-blocking.
| /* also read by content pinned below the bar, so it stays in sync */ | ||
| height: var(--dsr-nav-height); |
There was a problem hiding this comment.
[Claude Code 🤖] Nothing else reads --dsr-nav-height — it has exactly two references, the declaration on .detailed-style-reference and this height. Drop the second clause or point it at the consumer you have in mind. Non-blocking.
2 participants
Addresses CS-12672.
Navbar (theme dashboard)
Switchaffixed to the right end of the navbar in all four theme cardsnav/ulmarkup; nav clicks usehistory.replaceStateso embedded cards stop polluting host-app historySimpleNavBaris removed (drops a base-realm export)boxel-ui
BoxelDropdownno longer scroll-jumps on close (focus trap usespreventScroll, EBD's trigger refocus is skipped) — without this, menu actions can't smooth-scrollTheme card follow-ups
PreviewPillscomponent, reused in the brand guidecustomCssVariablesNarrow-container layout
minmax(min(N, 100%), 1fr),preblocks scroll instead of wrapping, brand guide palettes/rows/specimens wrap--boxel-sp; spacing uses--boxel-sp-*tokens instead ofcalc()🤖 Generated with Claude Code