Allow keyboard shortcuts

[Link2Twenty]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update

Description

Add handler for keyboard shortcuts. This allows custom shortcuts to be set up on a page by page (or even component by component) basis.

React demo

Usage

In this example we're going to run a function when the user presses CTRL+SHIFT+P

const shortcuts = {
  "ctrl+shift+KeyP":  (e) => {
    e.preventDefault();
    someFunction();
  }
}

<KeyboardShortcuts shortcuts={shortcuts} />

Related Tickets & Documents

related: #5023 #596

QA Instructions, Screenshots, Recordings

This facilitates changes in the future

Added tests?

• yes
• no, because they aren't needed
• no, because I need help

Added to documentation?

• docs.forem.com
• readme
• no documentation needed

[optional] Are there any post deployment tasks we need to perform?

Once this is merged I'd like to modify this current shortcut to use this method before thinking about what other shortcuts could/should be added.

[optional] What gif best describes this PR or how it makes you feel?

[rhymes]

Hi @Link2Twenty, interesting! What if we integrate https://github.com/github/hotkey ?

It'd be great if we'd eventually work towards a keyboard shortcuts modal populated by the registered shortcuts on each page, something like this:

[Link2Twenty]

It looks like hotkeys is only for interacting with specific elements. Whereas this way you can call any function you like including, but not limited to, dom interactions.

I liked how they registered their shortcuts, was much cleaner than my way so I've borrowed that.

[reobin]

Super clean @Link2Twenty !

I've implemented something similar in #10468 that is called KeyEventListener.

It would be nice to merge our efforts to go in the right direction with forem native shortcuts. My component doesn't handle key modifiers like ctrl and shift, which I think is a really nice touch.

I've already implemented KeyEventListener in a couple of places in the PR like the / and 0 shortcuts for the header, as well as j and k navigation for the article feed if you want to take a look.

Tell me what you think, feedback is appreciated! 🌞

Then, as @rhymes said, we could create an issue to work on a store that is fed by the calls to that very component. With that store, build a modal that lists all the available shortcuts for a given page.

[Link2Twenty]

Thanks @reobin 😊 I've updated my code to allow it to cope with single key shortcuts now, similar to hotkeys.

@nickytonline is this the sort of direction you'd like to see this go? If so we could get this merged then Reobin and I could look at using this in #10468

[citizen428]

@Link2Twenty I really like the work you and @reobin are doing on this! 😄

[nickytonline]

Just got out of the stream. Taking a peek now @Link2Twenty.

[nickytonline]

@Link2Twenty, thanks for the PR! This is looking good. Just some questions.

[nickytonline]

Thanks @reobin 😊 I've updated my code to allow it to cope with single key shortcuts now, similar to hotkeys.

@nickytonline is this the sort of direction you'd like to see this go? If so we could get this merged then Reobin and I could look at using this in #10468

@Link2Twenty I think going with use effect makes sense. I made a suggestion to consider making what you're proposing a custom hook. See PR review comments.

Update 10 minutes after writing first paragraph.😄 I saw @reobin's PR is using a custom hook, https://github.com/forem/forem/pull/10468/files#diff-976b57823cb7a305a55921032b2708aeR45, so I think a custom hook with useEffect is the direction to go.

[Link2Twenty]

@nickytonline any further thought or are we good to go?

[nickytonline]

This is looking good @Link2Twenty. I have some suggestions, but they do not block the PR.

One blocker though is we need some tests. If you need to see an example of testing hooks, take a peek at https://github.com/forem/forem/blob/master/app/javascript/utilities/__tests__/dragAndDrop.test.js and if you have any questions, let me know.

[nickytonline]

Thanks for being patient as there was a lot of back and forth and thanks so much for the PR @Link2Twenty! 🚀

[reobin]

Good job @Link2Twenty !

It seems stable enough. I'll start integrating this in #10468 some time tonight

[Link2Twenty]

@nickytonline thanks for the help hopefully I'll have a better understanding of tests now 😅

@reobin thank you, sorry for the hold up 😬

[reobin]

Quick question @nickytonline @Link2Twenty : is shared/components/ the right place for this hook? Asking because I'll put my list navigation component in the same place.

Edit: Was thinking more about utilities/ at first

[nickytonline]

Quick question @nickytonline @Link2Twenty : is shared/components the right place for this hook? Asking because I'll put my list navigation component in the same place.

@reobin, for common components shared/components is good place. We might revise this in the future, but for now, pop it there.

[Link2Twenty]

Just realised a problem with using event.code rather than event.key.toLowerCase()
In the uk the @ symbol would appear at shift+Quote but in the US it would be shift+Digit2.
I'm going to update the code so it can use either, because we may want crtl+Digit2 to do something but ctrl+Numpad2 to not. @reobin you shouldn't need to update what you've done I don't think.

[reobin]

Pulled the changes and all is well 👍

[Link2Twenty]

Hey @Ridhwana any progress on this? 😊

(no rush, of course, just curious)

[Ridhwana]

Looks great to me 🚀