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

Add onMapReady callback #1369

Merged
merged 3 commits into from
Jul 6, 2017

Conversation

Kerumen
Copy link
Contributor

@Kerumen Kerumen commented May 30, 2017

Fix #246

if (region) {
this.map.setNativeProps({ region });
} else if (initialRegion) {
this.map.setNativeProps({ region: initialRegion });
}
this._updateStyle();
if (onMapReady) onMapReady();
this.setState({ isReady: true });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should call onMapReady when state has changed. So like this:

this.setState({ isReady: true }, () => {
  if (onMapReady) onMapReady();
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason to why this is not being passed as an argument to onMapReady()?
refs are set in a arbitrary moment during react renders, and devs does not have control when they are set, thus outputting different results in some of initial renders. By passing the this as an argument that would be fixed and we would have the map ref exactly when the map is ready every time.

May I create a PR for this?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

@betoharres betoharres May 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rborn thanks for the reply. Unfortunately no arguments are being passed on that callback, I edited the file just by modifying that line to
if (onMapReady) onMapReady(this); and I got the result as I expected.

I've ran into this problem before and tried to workaround, but the results, as I said in my previous comment, are unstable; sometimes the ref is set, sometimes not.
My main goal is to run mapRef.animateToRegion(userLocation) when the map is ready, so that's why I'm interested on that change.

I have a PR ready to send if you want(besides, I'm just adding a word 😆 )

EDIT: also, let me know if there's a better approach to this problem, or if you want the argument to be different(passed as an object onMapReady({ref: this}))

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref should be set, if is not set you are doing something funny :)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reply again! I did some testing and you're right! If you only render the MapView component, it does have the ref set on every onMapReady() call, but if I add the useNetIfno() hook call to my component, the ref starts to return null sometimes when onMapReady fires.
If you are interested, I can open a issue and try to push a example project, because maybe something else could be interfering(even though I'm only rendering the Map with react-navigation).
I still like the idea of having an event available to set the ref, I'm not sure how it would affect the current code, but it doesn't seems to hurt I guess 😅

@cyakimov
Copy link

Seems that onMapReady never fires.

@freshnewegg
Copy link

onMapReady doesn't seem to fire here.

I have a one that works for RN 0.40 (the only difference is new RN's have ViewPropTypes and b4 we had View.propTypes)

freshnewegg@8e07955

Cheers,

@Kerumen
Copy link
Contributor Author

Kerumen commented Jun 15, 2017

It was only for the Android side. I will update the PR with the iOS part. Thanks @freshnewegg!

@viktorsec
Copy link

@Kerumen do you have an idea of how to fix this on iOS?

@Kerumen
Copy link
Contributor Author

Kerumen commented Jun 23, 2017

@viktorsec I've just updated the PR with iOS support! 🎉

@christopherdro
Copy link
Collaborator

Thanks for the PR!
This looks specific to use google maps on iOS. What is the behavior when using the default?

@Kerumen
Copy link
Contributor Author

Kerumen commented Jul 6, 2017

The callback will be called in the mapViewDidFinishRenderingMap delegate (source).

@christopherdro christopherdro merged commit 8a86e98 into react-native-maps:master Jul 6, 2017
yosimasu pushed a commit to yosimasu/react-native-maps that referenced this pull request Aug 7, 2017
* Add onMapReady callback

Fix react-native-maps#246

* Call onMapReady when state has changed

* Add onMapReady callback on iOS
sorodrigo pushed a commit to Vizzuality/react-native-maps that referenced this pull request Aug 21, 2017
* 'master' of https://github.com/airbnb/react-native-maps:
  v0.16.2
  Revert "Issue1176 improve ios marker performance by X100 (react-native-maps#1187)"
  Fix initial region android (react-native-maps#1563)
  v0.16.1
  Enhance Podfile. (react-native-maps#1252)
  Update marker component (react-native-maps#1428)
  Add legalNotice constant (react-native-maps#1458)
  Issue1176 improve ios marker performance by X100 (react-native-maps#1187)
  Fix initial region native prop (react-native-maps#1546)
  fix `Archive` configuration for iOS builds (react-native-maps#1550)
  v0.16.0
  Document MapView min/max zoom properties (react-native-maps#1538)
  Fix timing function used in AnimatedRegion.spring (react-native-maps#1479)
  Fix crashing the application when a user presses on the map and the Google Play Services need to be updated or at the moment of the process of updating (react-native-maps#1469)
  skip region monitoring if map object is null (react-native-maps#1443)
  Zoom level fixes (react-native-maps#1485)
  Attempt to fix crashes. A variant of react-native-maps#1403 but for another lifecycle method, as proposed by @Nelrohd. (react-native-maps#1464)
  Handle Android RN 0.47 breaking change (react-native-maps#1481)
  add MKTileOverlayRenderer (react-native-maps#1357)
  Add onMapReady callback (react-native-maps#1369)
pjaraherrera pushed a commit to pjaraherrera/react-native-maps that referenced this pull request Sep 27, 2017
* Add onMapReady callback

Fix react-native-maps#246

* Call onMapReady when state has changed

* Add onMapReady callback on iOS
nidongara pushed a commit to nidongara/react-native-maps that referenced this pull request Sep 27, 2017
* Add onMapReady callback

Fix react-native-maps#246

* Call onMapReady when state has changed

* Add onMapReady callback on iOS

(cherry picked from commit 8a86e98)
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.

9 participants