ENG-2113 Add footer action bar with open in new tab and split - #1292
Conversation
Obsidian renders modifiers as glyphs on macOS and as words on Windows and Linux. Roam's search footer hardcoded the macOS glyphs at each call site and showed the wrong hint on Windows (ENG-2000); routing every hint through one map is what keeps that from repeating. `formatHintKeys` takes `isMacOS` so the non-mac branch can be exercised without that platform. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Enter opens the active result in the current pane, Shift+Enter in a split, and both close the modal. Mod+Enter and Alt+Enter deliberately fall through, so the insert action (ENG-2114) can claim Mod+Enter as it does in Roam. The footer reuses Obsidian's own `prompt-instruction` markup, the classes `SuggestModal.setInstructions()` emits, so it matches the native quick switcher. This modal extends plain `Modal`, so that API is unavailable. Its actions are left-aligned rather than centred because they sit under a full-width result list. The Enter branch lives in the existing wrapper `onKeyDown`, which ENG-2109 moved off the input so result actions would have one place to live. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Replacing the page the user was already reading loses their place, which is the
opposite of what a lookup surface should do. `getLeaf("tab")` adds a tab to the
main panel instead, so the previous note stays open behind it.
This reuses the existing `openFileInNewTab`, so the `openFileInActivePane`
helper added earlier in this branch is no longer needed. The label now reads
"open in new tab" to match.
Diverges from the ticket's stated Solution, which specified `getLeaf(false)`.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
trangdoan982
changed the title
Obsidian's `prompt-instruction-command` is bold with no border, which made the lone `esc` hint read as emphasis rather than as a key. Roam's search footer draws every key as a bordered cap instead, so `esc` sits with the rest of the set. Keeps the `prompt-instructions` container for its native type and spacing, and takes the cap's border, radius, and background from Obsidian's CSS variables so it still follows the active theme. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Obsidian renders `aria-label` as a hover tooltip, so labelling the listbox meant a tooltip covered the results as soon as the pointer entered the list. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The close hint was the only footer item that ignored a click, which read as broken next to two working actions. It now goes through the same `FooterAction` as the others and calls the modal's own close. The badge carried both `title` and `aria-label` with the same text, so hovering one stacked a native tooltip on top of Obsidian's. Keeping `aria-label`, since Obsidian's is the themed one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@codex review |
![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: 005b5a2036
ℹ️ 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".
| event.preventDefault(); | ||
| moveActiveIndex(event.key === "ArrowDown" ? 1 : -1); | ||
| openActiveResult(event.shiftKey ? openFileInNewLeaf : openFileInNewTab); |
There was a problem hiding this comment.
When a keyboard user Tabs into a footer button, its bubbling Enter keydown is still intercepted here, preventDefault() suppresses the button's native click, and the selection shortcut runs instead. Consequently, Enter on the close button opens the active result in a new tab, while Enter on the split button also opens a new tab; ignore Enter events originating from footer buttons or scope this shortcut to the search input.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid, and worse than described now that close is also a button — fixed in 48c234e.
Enter on a focused footer button bubbled to the modal handler, whose `preventDefault()` suppressed the button's native click, so Tab→close and Tab→split both opened a new tab instead. Rather than scope the shortcut to the input (ENG-2109 deliberately moved it to the wrapper so arrow keys work anywhere in the modal), the Enter branch now returns early when the event originates inside a button:
if (event.target instanceof HTMLElement && event.target.closest("button") !== null) return;Arrow keys are unaffected — they return before this guard — and the buttons stay Tab-reachable.
| event.preventDefault(); | ||
| moveActiveIndex(event.key === "ArrowDown" ? 1 : -1); | ||
| openActiveResult(event.shiftKey ? openFileInNewLeaf : openFileInNewTab); |
| this footer sits under a full-width result list, so the actions line up with | ||
| its left edge instead. `flex-shrink` keeps the footer from collapsing inside | ||
| the fixed-height flex column above it. */ | ||
| .dg-node-search-modal .dg-search-footer { |
There was a problem hiding this comment.
Done in 48c234e — the footer layout is now utilities (inline-flex h-auto cursor-pointer items-center gap-1 rounded-none border-0 p-0 disabled:*, and shrink-0 justify-start px-0 pb-0 text-left on the row). Net 16 lines of CSS removed.
Four properties have to stay in CSS, and it is a specificity limit rather than preference: Obsidian sets color, background-color, and box-shadow in button:not(.clickable-icon) and button:hover, which are (0,1,1) and outrank a single utility class (0,1,0). I tried text-inherit first and it loses — the label renders in --text-normal on an interactive-grey pill. font-size: inherit stays because Tailwind has no inherit-size utility, and without it the button takes --font-ui-small instead of the row's smaller type. The remaining dg-search-footer-key rule is kept for the inherited font and em-based sizing.
A footer button reached by Tab had its bubbling Enter intercepted by the modal's keydown handler, whose preventDefault suppressed the button's native click. So Enter on close opened a new tab, and Enter on split opened a new tab too. Also moves the footer's layout onto Tailwind utilities, leaving only the four properties Obsidian defends with `button:not(.clickable-icon)` and `button:hover` — both (0,1,1), which outrank a single utility class — plus `font-size`, which has no inherit utility. Trims comments that explained history rather than the code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
01dc74a
into
eng-2109-create-node-search-modal-with-ranked-results-and-preview
2 participants
https://www.loom.com/share/85492db69f9f4c0d92f09d3234d64eb7
Scope check
$scope-checkagainst ENG-2113 and the final diff.Done When: two deliberate deviations, both cosmetic-to-small.Done Whenonly requires that "both actions open the correct note and close the modal", so this stays inside the acceptance boundary — but it contradicts the ticket'sSolution, which specifiedapp.workspace.getLeaf(false), and the ticket title. Replacing the note the user was already reading loses their place, which is the opposite of what a lookup surface should do. The footer label reads "open in new tab" to match.esc closehint. A third, non-clickable footer item for Escape, which Obsidian's modal scope already handles. Included so the footer matches Roam'sAdvancedSearchFooterand the native quick switcher, both of which show one.getLeaf(false)first would mean changing it immediately. (2) No, easily dropped.Solutionand title should be updated to say "new tab" rather than "active pane" — flagging rather than editing the ticket myself.Stacked on #1285 (ENG-2109) — review that first. Base is its branch, not
main.What this does
F8.
Enteropens the active result in a new tab in the main panel,Shift+Enteropens it in a split, both close the modal, and both are clickable in a new footer.Mod+EnterandAlt+Enterdeliberately fall through untouched so ENG-2114's insert-at-cursor can claimMod+Enter, the same combo Roam uses.Shift+Enterhas no native conflict: it means "create new file" only inside Obsidian's own quick switcher, which this modal does not offer.The Enter branch lives in the existing wrapper
onKeyDown— ENG-2109 moved that handler off the input specifically so result actions would have one place to live. It also guardsisComposing, so committing an IME candidate never opens a file.Both open helpers (
openFileInNewTab,openFileInNewLeaf) already existed and are reused unchanged, so no file outside the search feature is modified.Avoiding ENG-2000
Roam's footer hardcodes Blueprint's macOS glyph icons per action (
keyIcons={["key-option", "key-enter"]}), so Windows users see ⌘/⌥ while the handler actually accepts Ctrl. Rather than port that shape, every hint here goes through onekeyboardHints.tsmap keyed onPlatform.isMacOS. No call site can name a platform-specific symbol.formatHintKeystakesisMacOSas a parameter so the Windows/Linux branch is verifiable without that platform:Native styling
prompt-instructions/prompt-instruction/prompt-instruction-commandclasses — whatSuggestModal.setInstructions()emits.Beyond that it only needed
flex-shrink: 0(the.modal-contentcolumn is fixed-height withoverflow: hidden) and a reset for Obsidian's globalbuttonchrome.🤖 Generated with Claude Code