Skip to content

Conversation

thephoenixofthevoid
Copy link
Contributor

You can follow commit by commit to track changes

  1. Use ES6 default values syntax in config.ts
  2. Define DEFAULT_SWIPE_VELOCITY and DEFAULT_SWIPE_DISTANCE (magic numbers otherwise)
  3. reimplement useRecognizers using useMemo: simpler code and less hooks involved

@dbismut
Copy link
Collaborator

dbismut commented Apr 14, 2020

Thanks a million @thephoenixofthevoid, no one had looked into the code that deep I hope it isn't too ugly. I'm new to TypeScript and don't have any professional experience with React so there might be clumsy workarounds.

I've looked at your commits and they all look great, useRecognizer never felt right and I love the way you fixed it.

Is there any reason why you're using let vs const in config.ts? I know there's two sides on the matter, I don't have any strong opinion on it.

@thephoenixofthevoid
Copy link
Contributor Author

thephoenixofthevoid commented Apr 14, 2020

Just to be consistent among functions: in one function there is a need to change value, so it's impossible to use const in that case. Anyway they all turn into "var"s in the resulting code that will actually run.

I hadn't spent much time on that file yet, trying to avoid placing too many changes into a single PR. In the end, it's safer to use const everywhere, but some refactoring needs to be done to achieve that.

@dbismut
Copy link
Collaborator

dbismut commented Apr 14, 2020

No worries. Can I merge the PR or do you have more commits planned at this stage?

@thephoenixofthevoid
Copy link
Contributor Author

Yes. I will create a new PR for next changes.

@dbismut dbismut merged commit 0ae50fe into pmndrs:master Apr 15, 2020
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

Successfully merging this pull request may close these issues.

2 participants