Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

shortcut for parent action doesn't show palette #14

Open
Enteleform opened this issue May 21, 2022 · 12 comments
Open

shortcut for parent action doesn't show palette #14

Enteleform opened this issue May 21, 2022 · 12 comments

Comments

@Enteleform
Copy link

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.

@itaditya
Copy link
Owner

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

@Enteleform
Copy link
Author

Enteleform commented May 22, 2022

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.

@Enteleform
Copy link
Author

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

@itaditya
Copy link
Owner

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

@Enteleform
Copy link
Author

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?

@itaditya
Copy link
Owner

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 😄

@Enteleform
Copy link
Author

Enteleform commented May 22, 2022

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

@Enteleform
Copy link
Author

I just submitted PR #15

@Enteleform
Copy link
Author

🙌

SketchCommand-2.mp4

@itaditya
Copy link
Owner

awesome!!!

Regarding the E2E tests, I'll re-run them on my local once more to see if they are failing there too.

@Enteleform
Copy link
Author

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.

@declanchiu
Copy link

This happened to me too.

image

@itaditya

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants