Closed gforge closed 6 years ago
Let's try and get #147 merged in before this one. At first sight, it's all looking good
I would love your opinion on the file naming and structure. There are a few things that I've been thinking about:
key-handler-decorator
: Should probably just be decorator
utils.js
: This file is stuffed with functions. I personally prefer having a folder with an index file that merges all the functions.keyHandler
and keyToggleHandler
: These should perhaps be defined in the index.js
I've been using rollup
for some of my libraries and it allows more mjs
and other output formats that could be interesting optimizations: https://medium.com/@tarkus/how-to-build-and-publish-es6-modules-today-with-babel-and-rollup-4426d9c7ca71
Here's a basic rollup implementation: https://github.com/gforge/react-key-handler/tree/rollup
@gforge
I would love your opinion on the file naming and structure. There are a few things that I've been thinking about:
Let's discuss this in an issue or separate PR
Updated the PR with the fixes
@gforge can you rebase, would love to get this in for 1.2.0
Ok, rebased from master
Why the 5bcdd9a change? Since it's no official release I'd just revert that commit
The package updates were due to the failed Travis tests. This led to a rabbit hole that resulted in that I had to update babel. I figured it may be just as well since it has reached maturity and has the @ structure.
Thanks for merging! Sorry, didn't check babylon & parcel compatibility. Also, I like the prettier code style change :smile:
@gforge no worries, you did an epic job here! Just released v1.2.0-beta.3 so feel free to test it out
Thanks. I've tested it and it seems to work fine.
I think you should do a major release since the handling of simultaneous keycodes has changed
This PR does the following:
keyValue
/keyCode
to come in array formatcode
instead ofkeyCode
(also markskeyCode
as deprecated)code
index.js
is more informative, decorators have a separate file + moreThe PR includes all the updates in previous Flow PR (https://github.com/ayrton/react-key-handler/pull/147)
† The possibility of
keyValue
having priority is problematic in my opinion. If a user does not intend to use both then they probably wouldn't have specified them. If this truly should not be allowed then the constructor should throw an error. I can't see any use case where allowing all provided value hooks to trigger the event function.