Adjust Platform constants import #1211
Merged
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
drewandre
changed the title
jkadamczyk
suggested changes
Oct 16, 2020
There was a problem hiding this comment.
Hey @drewandre
I added a suggestion on how it can be implemented in case someone is using an older version and PlatformConstants is still available.
@jakub-gonet Please let me know if I'm wrong with the solution in any way 😄
This looks great, I didn't think that PlatformConstants may be used in older versions of this library. Thanks for the suggestion! Co-authored-by: Jakub Adamczyk <[email protected]>
|
@drewandre Thanks for the fixes, I'll try to look into this today and get it merged. Have a good day! |
|
E2E tests are kind of not working I have to investigate why 🤷 |
|
Updated branch with master. Sorry for introducing a bunch of unrelated lint changes. |
|
CI is failing due to missing |
jakub-gonet
changed the title
braincore
pushed a commit
to braincore/react-native-gesture-handler
that referenced
this pull request
Mar 4, 2021
## Description Fixes software-mansion#1210 by importing platform constants from react-native's `Platform` API instead of `NativeModules.PlatformConstants`. ForceTouchGestureHandler is the only handler that requires this file, so this change only affects this handler. It is also backward-compatible. I'm a bit confused why PlatformConstants would ever be used over Platform. They have a slightly different data structure, but as far as I can tell the constants are the same. ## Test plan I tested this on the [gesture-handler example app](https://github.com/software-mansion/react-native-gesture-handler/tree/master/Example) which is running RN v0.61.2, as well as the [reanimated v2 playground repo](https://github.com/software-mansion-labs/reanimated-2-playground) which runs RN v0.63.1. I did not test on the web, but PlatformConstants is not a file required by any web-specific files. There is a PlatformConstants.web.js file, but the getter defined in this file is the same data structure as exporting `Platform.constants` in PlatformConstants.js for mobile. Co-authored-by: Jakub Adamczyk <[email protected]> Co-authored-by: Jakub Gonet <[email protected]>
This was referenced Mar 13, 2021
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
Fixes #1210 by importing platform constants from react-native's
PlatformAPI instead ofNativeModules.PlatformConstants. ForceTouchGestureHandler is the only handler that requires this file, so this change only affects this handler. It is also backwards-compatible.I'm a bit confused why PlatformConstants would ever be used over Platform. They have a slightly different data structure, but as far as I can tell the constants are the same.
Test plan
I tested this on the gesture-handler example app which is running RN v0.61.2, as well as the reanimated v2 playground repo which runs RN v0.63.1. I did not test on web, but PlatformConstants is not a file required by any web-specific files. There is a PlatformConstants.web.js file, but the getter defined in this file is the same data structure as exporting
Platform.constantsin PlatformConstants.js for mobile.