shortcut for parent action doesn't show palette
Enteleform opened this issue · 12 comments
Example
defineAction({
id: "Pages",
title: "Pages",
shortcut: "$mod+g",
})
for(const {page, url} of pages){
defineAction({
parentActionId: "Pages",
id: page,
title: page,
run: (() => {window.location.href = url}),
})
}
Expected
Pressing ctrl + g
will show the command palette, immediately loading the Pages
list.
Actual
Pressing ctrl + g
does not show the command palette.
The Pages
list is loaded, but it is not visible until the command palette is toggled with ctrl + k
.
That's a nice catch. When I implemented parent actions I didn't consider that they could be triggered by keyboard shortcuts.
I'll need to look into whether we should change the types to prevent passing keyboard shortcut
or should we open the pallette with the parent action selected
So I just tried adding run
to the parent action and found that:
- the
run
action is executed - the palette closes and the list of children is not displayed
I think the parent actions might function more intuitively with a different shape & behavior. I propose:
run
is removedshortcut
loads the command palette at the parent level, displaying only its children- children can be hidden from the top-level menu, perhaps via the parent with an option like
isolateChildren
or something- I think
true
would be a good default for this, as it results in an organized nested hierarchy that can be traversed logically. Root-level actions can be defined as such, without using a parent action.
- I think
Here's the use case I'm currently building:
- creative coding sketchbook
- routes are structured to allow working through material like The Nature Of Code, with organized & ordered groups of exercises
- pages can be loaded via the palette, but there could potentially be tens or hundreds; so it is not ideal for them to be at the root of the palette, because there will be commands that are specific to the current sketch which are the priority for visual parsing.
SketchCommand.mp4
the way the API is designed is that a child action has the info that it's a nested action because it has parentActionId. However parent doesn't have any such info. So I went with the approach that parent actions should not have a run function because their only behaviour is to show its child actions.
So when you added a run function, the action no longer remained a parent action. I'll try to surface this in docs, JSDoc comments so it appears on hover.
Regarding only showing top level actions by default I think that makes sense. The reason it was like this was I wanted actions across all levels to be searchable. But since the model and view are decoupled I think this can be implemented.
In case you'd like to contribute, let me know
I can take a look at it. Can you point me in the direction of some good starting points for this?
What do you think about having a separate createParentAction
implementation to completely eliminate unexpected behaviors and suboptimal option combinations?
As I see there are two tasks
-
When user presses keyboard shortcut of a parent action, open the palette and select the action. For that you can look into this file. Since keyboard shortcuts and command palette interaction both trigger action, this is the common util called. Until now there was no difference between both invocations but maybe we can pass an
invokedBy: 'shortcut' | 'palette'
.
https://github.com/itaditya/solid-command-palette/blob/main/src/lib/actionUtils/actionUtils.ts#L31-L46 -
Let consumer choose between showing all actions or only top level actions (via Root prop). I think that absence of
run
function is a good enough indicator of parent action for now. For being top level it should also not have anyparentActionId
. A filter for such actions can be made in this file https://github.com/itaditya/solid-command-palette/blob/main/src/lib/createActionList.ts
I'm guessing by createParentAction
you mean a helper in this file only so that it's separate from other computations. If so, then yeah makes sense to me.
In case you meant just like we have defineAction
we should also have defineParentAction
I'm not so sure about that. Hope this helps. Feel free to ask more questions if you have doubts 😄
RE: defineParentAction
, yes that is what I was suggesting initially. I'll see what I can do without going that route if possible.
Also, I'm working on this now and ran into test failures after implementing the first task. I reverted the changes, and found that the tests fail even with no changes on my end. My OS is Windows 10, in case that has anything to do with it.
I just submitted PR #15
🙌
SketchCommand-2.mp4
awesome!!!
Regarding the E2E tests, I'll re-run them on my local once more to see if they are failing there too.
RE: E2E
I managed to run them successfully via the Playwright extension for VSCode. Dunno why they were failing via the CLI, maybe something to do with the additional browsers.