Add initial Windows implementation. #1409
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakub-gonet
requested changes
Mar 18, 2021
There was a problem hiding this comment.
Thanks for the effort.
I partially reviewed it but I have limited knowledge about C++ and RN bindings for it.
windows/RNGestureHandler/README.mdshould be in docs too.- We use spaces instead of tabs, please replace them.
- Why do we have 2 files like
RNGestureHandler62.sln,RNGestureHandler63.sln? I'm not familiar with VS, it was autogenerated with two RN versions? - Please use blocks even after one statement ifs.
windows/RNGestureHandler/RNGestureHandler/ReactPackageProvider.cpp
Outdated
Show resolved
Hide resolved
windows/RNGestureHandler/RNGestureHandler/RNGestureRegistry.cpp
Outdated
Show resolved
Hide resolved
tritao
force-pushed
the
windows
branch
2 times, most recently
from
8f73455
to
2fbd486
Compare
Thanks for the review, I have addressed your feedback and also fixed some other issues with the fling gesture as well as some edge cases in the end states for some gestures. Let me know if there is anything else and I'll be glad to fix it.
It needs different solutions depending on which react-native-windows version we are using. It's my understanding that the newer versions provide extra MSBuild files which we include for the automatic build setup. These are part of the template we have been using for bootstrapping these modules. |
|
Last push was to address a Yarn vs NPM issue on the lock file, should be good to go. |
|
Just realized I had missed some comments due to them being hidden by GitHub by default, pushed a fix. |
jakub-gonet
reviewed
Mar 23, 2021
There was a problem hiding this comment.
It needs different solutions depending on which react-native-windows version we are using. It's my understanding that the newer versions provide extra MSBuild files which we include for the automatic build setup. These are part of the template we have been using for bootstrapping these modules.
I see, it'd be nice to have this automated since I assume this only can be done from a Windows machine.
FYI: I'll need to test this implementation under windows so it may take some time. I'll also discuss this PR with our team.
One request from me: could you please avoid force pushing? We already use squash & merge and filtering by new commits is really helpful.
The solution files should never need to be modified going forward so I don't think this is a big concern.
Sure. |
|
Update: We want to merge it but I need to test it on my local machine before that. I'm planning to do that in the next week. |
|
Hi, sorry for the delay. I was unable to run rngallery project so I'm not able to check if your PR is working. Would you mind adding windows specific files and quick instruction to the Example README about generating those files? Also, I think README from windows/ directory should be moved directly into documentation, maybe on the installation page. Below is a list of compile errors from rngallery, maybe you know why those happen? |
|
I am not sure about those errors, but it seems like it is related to RNSketchCanvas. I also noticed you are compiling for ARM based on the output paths, that could be the issue.
Could you elaborate on this? I am not sure which files you refer to. By the way, for testing the PR, you need to launch the gallery example inside the UWP Simulator. You can switch to it as a device target in VS. |
|
@jakub-gonet I can try to help you get past the build errors with gallery. A couple of things to check....
|
|
I'm having some difficulty testing this on my local windows machine. I'm testing it through the example page in RNGallery - the solution is building fine and the examples are rendering as expected, however they don't seem to respond to touch interaction at all. I tried testing both on local machine and through the Simulator with no luck. @tritao Any ideas as to what might be going on? Were you able to get this working in RNGallery? |
|
Did you change the touch interaction modes in the Simulator side toolbar? It's expected that it won't do anything on local machine with mouse pointing device. You need to use the Simulator and select appropriate tool for each different gesture handler, see which ones I used for each on the screenshots. |
@tritao I did and still nothing. Also tried on my local machine that has a touch screen but that didn't seem to work either. |
2 tasks
|
I spent more time checking this and couldn't get it to work. Errors I'm getting in VS:
Things I tried:
|
jakub-gonet
added
the
Area: Gesture handlers
|
Facing similar issues. One thing I had to do was change: To: This is a patch that we've had to do on quite a few Microsoft react-native-windows library implementations. Wondering if there is a reason you are using MSBuildThisFileDirectory over SolutionDir. After fixing that, the error is now:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Adds initial support for react-native-windows.
The more advanced customization options for each gesture recognizer were not implemented as part of this initial pass.
They will be implemented later as needed.
Also this implementation is being done on behalf of Microsoft.
For context: microsoft/react-native-windows#4140
Test plan
Testing was done manually via the UWP simulator.
I've attached some screen recordings of the gestures working under the emulator.
These test examples will be added to react-native-gallery too.
Examples