imbhargav5 / rooks

Essential React custom hooks ⚓ to super charge your components!
https://rooks.vercel.app
MIT License
3.2k stars 215 forks source link

useKeys hooks for listening to one or multiple keypresses #62

Closed simbathesailor closed 5 years ago

simbathesailor commented 5 years ago

Feature

useKeys hooks allow adding certain action when one or multiple keys are pressed at the same time.

I have picked up the task of creating this hook.

I will be using this ticket for the questions and track the progress of this specific hook.

simbathesailor commented 5 years ago

@imbhargav5 Shall I cut the new branch from dev branch ? I have already done it, but thought of asking it once.

imbhargav5 commented 5 years ago

Hi @simbathesailor You can base of master. I have already merged dev branch into master. :)

simbathesailor commented 5 years ago

I implemented the working useKeysHook. So it basically works but doesn't work :). Here is the link for codepen where I am using the useKeysHook which I will be committing.

https://codepen.io/stack26/pen/BEwJqx?editors=1011

So how can I make sure the callback takes the updated value. In my case, i am looking for "testValue" variable that should get updated in the callback. I tried setting it to ref also but, I think something I am missing. Any suggestions ?

imbhargav5 commented 5 years ago

Do you mind creating a codesandbox link instead? Codepen is notoriously difficult for viewing large js files haha. Took a look but looks like useRef wasn't used to maintain ref to the current callback. I think that is the reason.

simbathesailor commented 5 years ago

Yes, you are right, didn't think that the code will become so big. :). Here is the codesand box link: https://codesandbox.io/s/p9q7q1l8y7 . I tried adding ref to callback also, but it didn't seem to work. Something it seems I am not clear about these hooks 😟 Now you can check, see how testValue is not updated on consecutive events.

imbhargav5 commented 5 years ago

Oh, how can I reproduce the bug?

simbathesailor commented 5 years ago

See the codesandbox link, there I have logged testValue in one of the callback. It should log the updated testValue, but it doesnot !!. See the first useKeys that i have used in index.tsx

imbhargav5 commented 5 years ago

Ah.. so some of the logic is against the hooks spec. For eg: you cannot have one hook within another. So useKeyDown within useEffect is wrong.

simbathesailor commented 5 years ago

You are right, now it seems to work, https://codesandbox.io/s/p9q7q1l8y7. But it triggers a thought that will it not be problematic to write interdependent hook more than one level ? 💭

Does everything look ok with the code now or you want to discuss anything, that we might change ? 🤔. Then I will sart adding the code into my rooks codebase.

imbhargav5 commented 5 years ago

So, I think it can be a lot less complicated than that to be honest. I think it can look at lot similar to useKeys with minor changes to make it multi.

simbathesailor commented 5 years ago

Hmm. you are right there. Made some changes after having a look at useKey. https://codesandbox.io/s/p9q7q1l8y7 Now the API look like :

useKeys(
    ["ControlLeft", "S"],
    event => {
      // Custom logic for handler
    },
    {
      target: documentRef,
      when: true
    }
  );

I have made use of event.code as the key array. Reson being small case and capital cases are same for this usecase. ctrl, alt, option like keys which are mostly two in keypads.Differtentiating betwen small case, uppercase, left , right buttons using event.key, event.location seemed not good. Using event.code will give the consumer of the hook to add letters, ControlLeft, AltLeft directly. you can play around with key code here https://keycode.info/

I had some issue with typescript. Can you have a look at it useKeys line 80. some typescript error is happening and i am having a hard time fixing it. I fixed it using MutableRefObject . But I wonder what is the difference between Ref and MutableRefObject ? After this i will try to raise pr and there we can review the code. Thanks

simbathesailor commented 5 years ago

Do let me know how it looks if by any chance you had the time to look at it. Then we can finalize it.

Also I wanted to know what are the check point before raising the pr ??

Like what kind of test cases are required ? Documenation ? Code comments doc blocks ?

simbathesailor commented 5 years ago

https://github.com/imbhargav5/rooks/pull/65