Meta+PageUp/PageDown increments the ViewerTab #3055
Conversation
🦋 Changeset detectedLatest commit: 2ac5be1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
||
| export type Shortcut = { | ||
| metaKey?: boolean; | ||
| shiftKey?: boolean; |
There was a problem hiding this comment.
didn't end up using, but figure we might later
| return true; | ||
| } | ||
|
|
||
| export function useGlobalShortcut(shortcut: Shortcut, act: () => void) { |
There was a problem hiding this comment.
I could remove this, as we have "useGlobalShortcut(s)" now, but I could imagine times when this would be a (bit) better (especially if useGlobalShortcut always needs memoization) . Not sure though.
There was a problem hiding this comment.
I like this hook, but can we express it by calling useGlobalShortcuts (possibly with useMemo for an array, though see my comment below)?
| return true; | ||
| } | ||
|
|
||
| export function useGlobalShortcut(shortcut: Shortcut, act: () => void) { |
There was a problem hiding this comment.
I like this hook, but can we express it by calling useGlobalShortcuts (possibly with useMemo for an array, though see my comment below)?
| if ( | ||
| (shortcut.shiftKey && !event.shiftKey) || | ||
| (shortcut.metaKey && !event.metaKey) || | ||
| event.key.toLowerCase() !== shortcut.key.toLowerCase() |
There was a problem hiding this comment.
Typically, if the shortcut is e.g. Cmd+1, then Cmd+Shift+1 or Cmd+Option+1 won't work. So it's not enough to check that the event has all modifiers that are required; you should also check that event doesn't have any extra modifiers.
It also means that you should check for altKey and ctrlKey here, which I forgot to do in the previous version.
|
|
||
| import { ViewerTab } from "../../lib/utility.js"; | ||
|
|
||
| const tabs = ["Imports", "Variables", "Exports", "Result", "AST"] as const; |
There was a problem hiding this comment.
This list is too far from the menu code and could easily get out of sync.
One solution that I like is to write a single custom hook, useViewerTabs:
const [viewerTab, setViewerTab] = useViewerTab({
output: simulation.output,
enableGlobalShortcuts: ...
})That hook would know the ViewerTab shape, so it would be easier to keep the list in sync.
There was a problem hiding this comment.
Hmm, that's not enough. Your code already has a problem that I can switch to non-existent items (we show "Variables", "Imports" and "Exports" conditionally).
Also, the source of truth for both which items are present and for the order of items is the dropdown menu, which is not even rendered fully on most cases.
The ideal solution is quite complicated, I obviously don't recommend you do it now, but it'd be something like:
Dropdown(or some specialized version of it) would expose an imperative handle withselectNext/selectPrev- which means action items would have to register themselves somehow on that list, with stable ids
selectNextandselectPrevmethods would have to callrender()(?) and analyze the list to detect the next value... and this part is especially messy and difficult to do
Ok, another idea:
- go with
useViewerTabidea above - return something like
{ viewerTab: ViewerTab, allViewerTabs: ViewerTab[], setViewerTab: (tab: ViewerTab) => void }from it - move the logic for detecting visible tabs from
<ViewerMenu>to this hook - now you can implement shortcuts in
useViewerTabcode correctly - add another component,
<ViewerMenuTabItem tab={viewerTab} output={output} /> - in
ViewerMenu'sDropdownMenu, renderallViewerTabs.map(tab => <ViewerMenuTabItem tab={tab} output={output} />
Uh, <DropdownMenuHeader>Debugging</DropdownMenuHeader> is a problem... I guess allViewerTabs could have sections, but the boundary between hooks and components could get awkward.
| () => [shortcut, act], | ||
| [shortcut, act] | ||
| ); | ||
| useGlobalShortcuts([shortCut]); |
There was a problem hiding this comment.
[shortCut] creates a new array every time, so memoization is not working. Should be useMemo(() => [[shortcut, act]], ...)
It would be nice to focus() on the first item in the tab, but that state is all the way in the ViewerProvider. Getting this to all work would be a bit annoying, so leaving for now.