itaditya/solid-command-palette

shortcut for parent action doesn't show palette

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 removed
  • shortcut 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.

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

  1. 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

  2. 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 any parentActionId. 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.

image

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.

This happened to me too.

image

@itaditya