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

Pass camera move's reason to JS callbacks #3358

Merged
merged 5 commits into from Oct 14, 2020

Conversation

yairopro
Copy link
Contributor

@yairopro yairopro commented Mar 25, 2020

Does any other open PR do the same thing?

There is a PR that solves likely the problem #2935, but with a workaround.
The problem we are trying to solve is to know the reason when the region has changed on the map.
Using onPanDrag, implemented by the PR #2935, is a non stable workaround. Because will would have to first store somewhere a boolean on the onPanDrag, and use it once onRegionChangeComplete.

What issue is this PR fixing?

Issue #1620 and #2756.

How did you test this PR?

I implemented and tested (on a real device) the code for android. The iOS part too has been implemented by my friend. And was tested on an emulator.

@yairopro
Copy link
Contributor Author

yairopro commented Mar 26, 2020

To pass the enum value to the JS callbacks onRegionChange & onRegionChangeComplete, I first added the property reason to the android RegionChangeEvent. And once the event received on the JS thread, I pass a 2nd object parameter, called details, to the callback, like onRegionChange(region, details). I build the details object before calling the callback.
For now, details has only one property reason. In the future it may have other properties, which like reason won't have their place in the Region object.

@yairopro
Copy link
Contributor Author

yairopro commented Apr 1, 2020

Since on iOS the map gives only a boolean, if it's a user gesture or not, then I adapted the same behaviour for android (it converts the integer to the same boolean) and renamed the property from reason to isGesture.

@rborn
Copy link
Collaborator

rborn commented Apr 1, 2020

LGTM @christopherdro 🐽

@yairopro 😻

@rborn
Copy link
Collaborator

rborn commented Oct 1, 2020

@yairopro I know this is a very old PR 😅 but could you add the extra info in the docs too (the events seem to be missing the details param ?
Then I'll merge 🙂

@yairopro
Copy link
Contributor Author

yairopro commented Oct 1, 2020

Oh you're right. I've forgotten the documentation.
Let me some time to re-read the PR code.

@yairopro
Copy link
Contributor Author

@rborn I updated the documentation.
You can edit it too if you wish.

@rborn rborn merged commit 84cd0c4 into react-native-maps:master Oct 14, 2020
@rborn
Copy link
Collaborator

rborn commented Oct 14, 2020

😻

@yairopro
Copy link
Contributor Author

🥳 Thanks

@rennehir
Copy link

Nice feature, this would really help us a lot with our product!

But I noticed that there hasn't been a release since March. @rborn @yairopro do you have any estimates when that might occur? Or should I just point my package.json to a specific commit?

Cheers, great work! 🥳

@yairopro
Copy link
Contributor Author

Hi @rennehir
I can't say, I am just an outside contributor.

@juice49
Copy link
Contributor

juice49 commented Oct 22, 2020

This doesn't seem to be working with Apple Maps on iOS. I logged the nativeEvent variable in _onChange, and the isGesture property is missing:

{
  "region": {
    "longitude": -2.7246242753611227,
    "latitude": 54.88447300842492,
    "latitudeDelta": 4.24778519660066,
    "longitudeDelta": 4.548340505366269
  },
  "target": 989,
  "continuous": true
}

I get the property as expected on Android, for example:

{
  "region": {
    "longitudeDelta": 4.520089812576771,
    "longitude": -2.9489495046436787,
    "latitudeDelta": 3.685225103923095,
    "latitude": 55.22018295561497
  },
  "isGesture": true,
  "continuous": true
}

I haven't tested this with Google Maps on iOS.

@yairopro
Copy link
Contributor Author

yairopro commented Oct 23, 2020

Hi @juice49
The flag comes only with google map (iOS too).
I should have say that in the doc, my bad.

@juice49
Copy link
Contributor

juice49 commented Oct 23, 2020

Thanks @yairopro! I wasn't sure if this should work or not.

@rborn
Copy link
Collaborator

rborn commented Oct 23, 2020

@juice49 mind a PR with the doc change ? 😍

@juice49
Copy link
Contributor

juice49 commented Oct 23, 2020

@rborn Absolutely! PRd in #3589. Thank you for your work maintaining this excellent library 🙂.

@rborn
Copy link
Collaborator

rborn commented Oct 23, 2020

@juice49 merged 😻

@juice49
Copy link
Contributor

juice49 commented Oct 23, 2020

Thanks @rborn!

@RyanTG
Copy link

RyanTG commented Jun 17, 2021

Sorry - earlier today I commented on the commit rather than the issue. I thus deleted the comment, and here it is in a more appropriate place:

Just in case the developers of this commit missed it, some of us are not able to get this to work in version 0.28.0. isGesture is undefined. Any guidance?
#3818

@christopherdro
Copy link
Collaborator

Looking through the source it appears the isGesture property is only available on google maps. If you are using Apple Maps then the value will be undefined.

@RyanTG
Copy link

RyanTG commented Jun 17, 2021

We are using Google Maps on both iOS and Android, and version 0.28.0. It's unclear if we're doing something wrong (and it's working for others) or if there is an issue with the feature.

RyanTG referenced this pull request in expo/expo Jun 18, 2021
expo-ads-admob@10.1.0
expo-ads-facebook@10.1.0
expo-analytics-amplitude@10.2.0
expo-analytics-segment@10.2.0
expo-app-auth@10.2.0
expo-app-loading@1.1.0
expo-apple-authentication@3.2.0
expo-application@3.2.0
expo-auth-session@3.3.0
expo-av@9.2.0
expo-background-fetch@9.2.0
expo-barcode-scanner@10.2.0
expo-battery@5.0.0
expo-branch@4.2.0
expo-brightness@9.2.0
expo-calendar@9.2.0
expo-camera@11.1.1
expo-cellular@3.2.0
expo-clipboard@1.1.0
expo-constants@11.0.0
expo-contacts@9.2.0
expo-crypto@9.2.0
expo-dev-launcher@0.5.1
expo-device@3.3.0
expo-document-picker@9.2.0
expo-error-recovery@2.2.0
expo-face-detector@10.1.0
expo-facebook@11.2.0
expo-file-system@11.1.0
expo-firebase-analytics@4.1.0
expo-firebase-core@3.1.0
expo-firebase-recaptcha@1.4.2
expo-font@9.2.0
expo-gl-cpp@10.4.0
expo-gl@10.4.0
expo-google-app-auth@8.2.0
expo-google-sign-in@9.2.0
expo-haptics@10.1.0
expo-image-loader@2.2.0
expo-image-manipulator@9.2.0
expo-image-picker@10.2.0
expo-image@1.0.0-alpha.1
expo-in-app-purchases@10.2.0
expo-intent-launcher@9.1.0
expo-keep-awake@9.2.0
expo-linear-gradient@9.2.0
expo-linking@2.3.0
expo-local-authentication@11.1.0
expo-localization@10.2.0
expo-location@12.1.0
expo-mail-composer@10.2.0
expo-media-library@12.1.0
expo-module-template@9.1.0
expo-modules-autolinking@0.0.2
expo-modules-core@0.1.1
expo-network@3.2.0
expo-notifications@0.12.0
expo-payments-stripe@10.0.0
expo-permissions@12.1.0
expo-print@10.2.0
expo-random@11.2.0
expo-screen-capture@3.2.0
expo-screen-orientation@3.2.0
expo-secure-store@10.2.0
expo-sensors@10.2.0
expo-sharing@9.2.0
expo-sms@9.2.0
expo-speech@9.2.0
expo-splash-screen@0.11.0
expo-sqlite@9.2.0
expo-standard-web-crypto@1.1.0
expo-store-review@4.1.0
expo-task-manager@9.2.0
expo-tracking-transparency@1.1.0
expo-updates@0.7.0
expo-video-thumbnails@5.2.0
expo-web-browser@9.2.0
expo-yarn-workspaces@1.5.2
expo@42.0.0-alpha.0
jest-expo-enzyme@1.2.0
jest-expo-puppeteer@1.1.0
jest-expo@42.0.0
react-native-unimodules@0.14.1
unimodules-app-loader@2.2.0
unimodules-task-manager-interface@6.2.0
unimodules-test-core@0.4.0
@RyanTG
Copy link

RyanTG commented Jul 7, 2021

@yairopro I just want to say thanks for the isGesture feature. Looking back at the issues on this repo, it's been brought up at least half a dozen times over the past few years - so it clearly solves a much-needed issue! It works great, and it saved our butts.

It would be cool to have this functionality on Apple Maps, too. Is there any chance of that? In our particular use case, now that an upcoming version of react-native-maps is going to include a manual theme toggle for Apple Maps, there is feature parity between the two (aside from isGesture).

@yairopro
Copy link
Contributor Author

yairopro commented Jul 8, 2021

Hi @RyanTG
😁 I'm really happy to hear that my work helps. And I really thank you for this warming message.

But unfortunately, I am not able to help you since I am not an iOS developer. sorry
The iOS code in this PR has been added by my colleague who I asked to help me once for this PR but he's not used to open-source development. I just gave him instructions how to implement the feature in objective-c after I've written it on the other side for Android in java, and the data-flow was the same.

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.

None yet

6 participants