leastbad / stimulus-hotkeys

MIT License
134 stars 9 forks source link

Support stimulus action options #17

Closed norkunas closed 1 year ago

norkunas commented 1 year ago

Would be nice if could add :prevent like on data-action. Ref: https://stimulus.hotwired.dev/reference/actions#options

For example data-hotkeys-bindings-value='{"ctrl+k": "#search-dialog->dialog#open:prevent"}' to prevent focusing browser's search bar.

leastbad commented 1 year ago

Great idea! Do you have a PR for me?

norkunas commented 1 year ago

Sorry, but no :) To parse the options is easy part, but don't know how to later call them

leastbad commented 1 year ago

Could something like this work?

  map = binding => {
    try {
      let [key, value] = binding
      const prevent = value.includes(':prevent')
      value = value.replace(/:prevent/, '')

      // ...

      return prevent
        ? [key, event => {
          event.preventDefault()
          controller[method].bind(controller, ...args)()
        }]
        : [key, controller[method].bind(controller, ...args)]

That is pseudocode, but I suspect it's pretty close to what you'd have to do to get it working. I encourage you to clone the repo and try it out locally!

Note, the ternary might not even be necessary. Could it make sense to call preventDefault on every keypress? https://github.com/jaywcjlove/hotkeys doesn't explicitly address it in the README, and I'm not a keypress event expert.

norkunas commented 1 year ago

it will, if we want to support only the :prevent option, stimulus has also other options.

Could it make sense to call preventDefault on every keypress?

but then it would behave a little different from the data-action unless you dont strictly align with it?

norkunas commented 1 year ago

But maybe other options doesn't make sense here :)

leastbad commented 1 year ago

Hey, you asked about :prevent, presumably because this is functionality you need. I am not on a mission to reimplement (or track future changes to) the Stimulus action API.

I agree that preventing default actions is a reasonable thing to want for this library, even if I'm way too busy to tackle it myself right now. I also like expanding the pool of OSS contributors, so again, I gave you the solution. You just have to test it, tweak it, make a PR.

I don't know why we would support :passive, :once or :self. They don't seem to apply at all to keypress captures.

leastbad commented 1 year ago

Anyhow, like I said... the idea/possibility/wisdom of calling preventDefault for every keypress capture is very much an open question. Please, do some research and we'll make this library even better, together.

What I don't want is to make it the default and then break the library because everyone knows you're only supposed to preventDefault in certain scenarios...