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

Doesn't use React's event system #55

Open
greena13 opened this issue Oct 23, 2017 · 2 comments
Open

Doesn't use React's event system #55

greena13 opened this issue Oct 23, 2017 · 2 comments

Comments

@greena13
Copy link

react-shortcuts does not seem to use react's synthetic event system.

This seems to be the underlying cause of confusion for people who create issues.

It may also be the reason it interferes with some of React's normal events (#54).

It also makes testing more difficult as the normal enzyme event simulation helpers don't seem to always work with how react-shortcuts functions.

react-shortcuts should, if possible, hook into the synthetic events and bubbling that React provides, to offer more predictable and testable behaviour (and maybe remove the need for one or two props).

@petrbrzek
Copy link
Member

This is true, react-shortcuts uses combokeys module which is basically a clone of mousetrap package. Although I agree it would be better to use React's synthetic event it would mean to completely remove combokeys and write the logic that handles keys (easy part) and sequences (harder) from scratch and also write specs for various browsers. So this is quite a large change. Also, there are some technical questions of how would prop global and targetNodeSelector work. But yeah, we could remove isolate which is confusing and hacky.

@greena13
Copy link
Author

Thanks for giving me some insight into what changes this would entail.

I guess sequences would need to be implemented using some sort of state machine that instantiated all possible states from the declared keyboard shortcuts.

I am not sure that global would be required if the events were implemented with react synthetic events. As described in #56, I think global shortcuts should be defined at the top of the application and called last. This is how React's event propagation works, anyway.

targetNodeSelector is a bit trickier. It seems a little antithetical to React's paradigm. Could you list the reasons someone may want to use this prop? Perhaps there would be alternatives in most or all of those cases that would remove the need for the prop at all.

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

2 participants