support keydownGlobal

[sheldonrong]

WIP or POC

I don't have unit test at this stage, but initial testing says it is good.

This is to address issue #69

Also, formattring kinda screw up, but if you are happy with the concept, I can fix these trivial issues :)

[glortho]

@sheldonrong could you revert the formatting changes? it makes it a bit hard to grok what actually changed.

[sheldonrong]

@glortho no problem, ill work on it today.

[sheldonrong]

@glortho updated.

Since we use TypeScript, I also wrote a simple type definition file (not in this commit), not sure if you take it or not. I can submit it to @types/react-keydown @npm if you prefer type definition to be in a separate repo.

[glortho]

thank you @sheldonrong! this is looking good, though i have a few questions for us to discuss (cc @salmanm):

1. Do we want local bindings to take precedence over global bindings, vice versa, or should both of them should fire? I believe with your solution here, if you have both a global binding and a local binding with the same keys, the component that was most recently focused will still get precedence. For predictability my vote is that we should probably choose one -- local always beats global, global always beats local, or both fire. What do you think?

2. Depending on the answer to 👆 would it be easier to track global bindings separately from local ones? I'm not sure. Maybe we just need to answer the first question first.

3. Should global bindings be triggered in <input>, <select>, and <textarea>, or should they be subject to the same restrictions as local bindings? It seems maybe like we will eventually want this to be configurable per binding as with debounce.

[glortho]

And thank you for creating a type def @sheldonrong ! I think it's probably good to just include it here since it's so small.

[sheldonrong]

I'm voting for firing both at this stage. I think if developer opt for using @keydownGlobal need to reckon that the event would be triggered regardless if the component is focused.

I haven't tried when using @keydown and @keydownGlobal with the same key yet as I can't think of a use case.

In our case, we have a navigation header that is sticky at the top of the app, which I need to use the @keydownGlobal because rest of the pages haven't been fully converted to React app yet.

[glortho]

You're right, devs ideally shouldn't have duplicate bindings across local + global but good to have consistency either way. So I think what we need then is to continue through the loop here https://github.com/glortho/react-keydown/pull/71/files#diff-3bf35cd41da51153dcca3003656f7c67R90 to potentially match multiple bindings - exactly one local binding (the most recently activated one), and one or more global bindings.

Does that sound right?

[sheldonrong]

ill think about it and get back to you tomorrow :)

[salmanm]

Minor: All empty class decorators will create a new object. Perhaps we should have a defaultOptions above and just use that if not supplied.

(Is find if you don't do it now, I'll add when I work on debounce)

Edit: I see that you're not taking options from the user, so its fine for now but will need to be changed.

[salmanm]

This will clash a bit when we start taking options from the decorator. options will then be last arg.

[salmanm]

@glortho

1 & 2) I think keydownGlobal is mostly needed when you want it to fire even when your component does not have focus. But then if we have a local contender, we should fire this first and then if key matches with the global one's fire them all. However, its important that we should be able to know in the global handler that it has been handled already. So we could have conditions there to "act" only if it hasn't been acted upon. And/or trigger of global should probably depend on the local's preventDefault(). So yes, we should track global bindings separately from local ones.

A hypothetical case could be, say I have a modal popup with Escape key bound to close it. The popup shows a grid with some data in it. The grid is editable and on overtyping, I show the input and take user's input, If I have and Escape key there too for hiding the input. It will also close the popup.

3. I'd say same restrictions should apply there too. No trigger for <input>, <select>, and <textarea> are focused