-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Support Google Maps on iOS #548
Conversation
Looks like you need to rebase. |
@@ -33,3 +33,4 @@ local.properties | |||
# | |||
node_modules/ | |||
npm-debug.log | |||
Pods/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add trailing newline
Wow this looks interesting :)! Thanks for making an early PR! |
The PR title kind of confused me. Maybe rename it to "Google Maps on iOS"? 😉 |
Far from feature parity, but I think that this is ready. |
when feature release? @gilbox |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow. This was a ton of work and is looking great! Overall, the architecture of all of this seems good to me. I had several pretty small changes that I'm hoping you can make, then after that I think we will be ready to merge!
|
||
const viewConfig = { | ||
uiViewClassName: 'AIRMap', | ||
uiViewClassName: 'AIR?Map', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are using one component for two possible different bridged components, this would be AIRMap
or AIRGoogleMap
. I added the ?
to indicate to the user that we don't know which one it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than using the arbitrary name in the viewConfig
, how about we actually add the correct viewConfig
to the component in decorateMapComponent
?
*/ | ||
mapProvider: PropTypes.oneOf([ | ||
null, | ||
undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this needed? Doesn't not having .isRequired
on the end of this already allow these values?
import { | ||
contextTypes, | ||
airMapName, | ||
AIRGoogleMapIsInstalled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get rid of the AIR
prefix here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we do this before merging? I don't think AIR
prefixes should really make it to JS user-land code...
const airMaps = { | ||
default: nativeComponent('AIRMap'), | ||
}; | ||
if (Platform.OS === 'android') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Platform.select
might be cleaner here...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it would mean evaluating both cases
airMaps.google = airMaps.default; | ||
} else { | ||
airMaps.google = AIRGoogleMapIsInstalled ? nativeComponent('AIRGoogleMap') : | ||
createNotSupportedComponent('react-native-maps: AirGoogleMaps dir must be added to your xCode project to support GoogleMaps on iOS.'); // eslint-disable-line max-len |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's bring in the dedent
module and use a template string for this. I've been using that in some other places and it is really nice.
* Any value other than "google" will default to using | ||
* MapKit in iOS or GoogleMaps in android as the map provider. | ||
*/ | ||
mapProvider: PropTypes.oneOf([ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we rename this to just provider
instead of mapProvider
?
@implementation AppDelegate | ||
|
||
- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions | ||
{ | ||
NSURL *jsCodeLocation; | ||
|
||
[GMSServices provideAPIKey:@"AIzaSyAeHIC4IG7XKT2Ls5Ti_YZV-6DHQk6dVHE"]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i assume this is safe to put in here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure it's safe. However, this is a key I generated from my personal account. Do you want to use a key from whatever account you used to generate the android key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's use the same key we're using for the Android example app:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spikebrehm that key didn't work because it's not configured to allow this iOS app
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. Would the key you generated work with Android? It would be nice to just have one key for the whole example app.
use_frameworks! | ||
|
||
target 'AirMapsExplorer' do | ||
pod 'React', path: '../node_modules/react-native', :subspecs => [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strange indenting
marker.realMarker.map = nil; | ||
[self.markers removeObject:marker]; | ||
} | ||
[_reactSubviews removeObject:(UIView *)subview]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if we call [super removeReactSubview:subview]
are we intentionally not calling it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're also not calling [super insertReactSubview:subview]
above. I'm not sure the best way to handle this, but markers aren't added the way normal views are added as subviews.
CLLocationCoordinate2D b = CLLocationCoordinate2DMake(initialRegion.center.latitude - latitudeDelta, | ||
initialRegion.center.longitude - longitudeDelta); | ||
GMSCoordinateBounds *bounds = [[GMSCoordinateBounds alloc] initWithCoordinate:a coordinate:b]; | ||
GMSCameraPosition *camera = [self cameraForBounds:bounds insets:UIEdgeInsetsZero]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it really seems like we should have a pulled out and reusable function to convert an MKCoordinateRegion
to a GMSCoordinateBounds
export const USES_DEFAULT_IMPLEMENTATION = 'USES_DEFAULT_IMPLEMENTATION'; | ||
export const NOT_SUPPORTED = 'NOT_SUPPORTED'; | ||
|
||
export function airMapName(mapProvider) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we prefix each of these getters with get
? It's nice for functions to be named as verbs, and non-function variables to be nouns.
|
||
const viewConfig = { | ||
uiViewClassName: 'AIRMap', | ||
uiViewClassName: 'AIR?Map', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than using the arbitrary name in the viewConfig
, how about we actually add the correct viewConfig
to the component in decorateMapComponent
?
|
||
export const AIRGoogleMapIsInstalled = !!NativeModules.UIManager[airMapName('google')]; | ||
|
||
export function decorateMapComponent(Component, { componentType, providers }) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about rather than naming this file common.js
, this is decorateMapComponent.js
, and we do:
export default function decorateMapComponent(Component, { componentType, providers }) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I agree. I don't like naming the file common.js
export const NOT_SUPPORTED = 'NOT_SUPPORTED'; | ||
|
||
export function airMapName(mapProvider) { | ||
if (mapProvider === 'google') return 'AIRGoogleMap'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's pull the providers out to constants, i.e. PROVIDER_GOOGLE
.
mapProvider: PropTypes.string, | ||
}; | ||
|
||
export const createNotSupportedComponent = message => () => console.error(message) || null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why || null
? Is this just a clever shorthand for:
() => {
console.error(message);
return null;
}
NativeModules.AIRMapManager[name].apply( | ||
NativeModules.AIRMapManager[name], | ||
this._mapManagerCommand(name).apply( | ||
this._mapManagerCommand(name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit fishy. It calls the method with that same method that's being called as the this
context?
@@ -468,14 +499,25 @@ class MapView extends React.Component { | |||
|
|||
MapView.propTypes = propTypes; | |||
MapView.viewConfig = viewConfig; | |||
MapView.childContextTypes = contextTypes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Name this local variable childContextTypes
.
@@ -56,18 +58,36 @@ class App extends React.Component { | |||
); | |||
} | |||
|
|||
renderGoogleSwitch() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only render this on iOS, right?
[Callouts, 'Custom Callouts'], | ||
[Overlays, 'Circles, Polygons, and Polylines (ios error)'], | ||
[DefaultMarkers, 'Default Markers'], | ||
[TakeSnapshot, 'Take Snapshot (incomplete)'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps instead of rendering a different list of examples, what do you think about adding a third element to the array indicating whether or not Google Maps is supported, and I guess also a fourth element indicating google-specific caption?
this.renderExamples([
[StaticMap, 'Static map', true],
[DisplayLatLng, 'Tracking Position', true, '(incomplete)'],
// ...
[PolylineCreator, 'Polyline Creator', false],
]);
style={StyleSheet.absoluteFill} | ||
contentContainerStyle={styles.scrollview} | ||
> | ||
<Text>Clicking</Text> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why put each of these in its own <Text>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh... to make it long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, thanks for your hard work, @gilbox! This is great. 🍻
At some point can you provide a list of TODOs for what features are not yet working, or what needs to be followed-up on to achieve parity with MapKit?
- GoogleMaps/Base (2.0.1) | ||
- GoogleMaps/Maps (2.0.1): | ||
- GoogleMaps/Base (= 2.0.1) | ||
- React/Core (0.32.1): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Elsewhere in the codebase expects 0.32.0 to be installed. Can you make these 0.32.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I will change the version in examples/package.json
from ^0.32.0
--> 0.32.0
so this doesn't happen.
{ | ||
AIRGoogleMapCallout *callout = [AIRGoogleMapCallout new]; | ||
// UITapGestureRecognizer *tapGestureRecognizer = [[UITapGestureRecognizer alloc] initWithTarget:self action:@selector(_handleTap:)]; | ||
// // setting this to NO allows the parent MapView to continue receiving marker selection events |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to delete this since I'm not clear on the intended behavior.
RCT_EXPORT_VIEW_PROPERTY(region, MKCoordinateRegion) | ||
RCT_EXPORT_VIEW_PROPERTY(showsBuildings, BOOL) | ||
RCT_EXPORT_VIEW_PROPERTY(showsCompass, BOOL) | ||
//RCT_EXPORT_VIEW_PROPERTY(showsScale, BOOL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not supported?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct, it's not supported.
@interface AIRGoogleMapMarker : UIView | ||
|
||
@property (nonatomic, weak) RCTBridge *bridge; | ||
//@property (nonatomic, weak) AIRGoogleMap *map; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
delete?
// AirMaps | ||
// | ||
// Created by Gil Birman on 9/2/16. | ||
// Copyright © 2016 Christopher. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who's Christopher?
const components = { | ||
default: requireNativeComponent(getAirComponentName(null, componentType), Component), | ||
}; | ||
let components = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be const
|
||
Component.contextTypes = contextTypes; | ||
Component.prototype.getAirComponent = function() { | ||
const provider = this.context.provider || PROVIDER_DEFAULT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Up here could we just
if (provider === PROVIDER_DEFAULT) {
return getDefaultComponent();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the problem with this is it will cause us to call getDefaultComponent
2x instead of 1x
class App extends React.Component { | ||
constructor(props) { | ||
super(props); | ||
|
||
this.state = { | ||
Component: null, | ||
useGoogleMaps: false, | ||
showGoogleMapsSwitch: Platform.OS === 'ios', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal, but this doesn't have to be in this.state
because it never changes.
// AirMaps | ||
// | ||
// Created by Gil Birman on 9/2/16. | ||
// Copyright © 2016 Christopher. All rights reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each of these files has the phantom "Christopher" copyright.
export const USES_DEFAULT_IMPLEMENTATION = 'USES_DEFAULT_IMPLEMENTATION'; | ||
export const NOT_SUPPORTED = 'NOT_SUPPORTED'; | ||
|
||
export const PROVIDER_DEFAULT = 'default'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's also export these on MapView
, so people can do:
<MapView provider={MapView.PROVIDER_GOOGLE}>
...
</MapView>
Perhaps we should pull these out to a ProviderConstants.js
or similar.
export const googleMapIsInstalled = !!NativeModules.UIManager[getAirMapName(PROVIDER_GOOGLE)]; | ||
|
||
export default function decorateMapComponent(Component, { componentType, providers }) { | ||
let components = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be const
Smoke tested on iOS, it works great!!! |
I think I've addressed all the comments that I could. This code review system is way confusing compared to GHE. |
@@ -0,0 +1,81 @@ | |||
/* eslint-disable */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not disable this for the whole file. Can we just fix the violations?
@@ -67,6 +68,10 @@ class AnimatedMarkers extends React.Component { | |||
} | |||
} | |||
|
|||
AnimatedMarkers.propTypes = { | |||
provider: PropTypes.string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally this would specify PropTypes.oneOf([...])
, but NBD really.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps we could expose MapView.ProviderPropType
!!
AMAZING. I came across the Google Places API policies and I saw I can't put Places data on other than Google's maps so this PR saves my life and the life of our iOS version. Can we hope to see these additions landing in the coming month? :D |
Wooo 🎉 🍻 let's do this! |
In #548, we added Google Maps support for iOS. We changed the example app to list examples based on whether they support Google Maps on iOS. The logic was such that on Android it only showed the examples that are iOS + Google Map compatible, however on Android this condition is irrelevant, this commit changes it to show all examples on Android, regardless.
This was unexpected haha 🎉 🍻 congrats |
* commit '7a5083cbb820faec0bb684c010e15c222ce7e533': (22 commits) Add an options argument Update README with example Add edgePadding example Add default arguments to fitToCoordinates -Set constant for baseMapPadding used in newLatLngBounds calls -Remove extraenous comment -Port fitToCoordinates functionality over to Android Add example Update docs Add fitToCoordinates method Fix list of examples on Android Support Google Maps on iOS (react-native-maps#548) fix react-native-maps#453 Make MAP_TYPES constant, don't pass `none` mapType to iOS Fix some changes missed in rebase renamed the 'url' property to 'urlTemplate' for consistency with ios added support for tile overlays (MKTileOverlay) changed url template to use {x} {y} {z} pattern replacement initial support for tile overlays Updated Mapview onRegionChange events notes Adding support for Android lite mode ...
@gilbox @spikebrehm is it possible to use it without cocoapods? |
Has anyone noticed a performance regression on Android with this change? I'm still investigating, but it seems like calling |
@stereodenis unfortunately GoogleMaps SDK for iOS only supports install with cocoapods |
In react-native-maps#548, we added Google Maps support for iOS. We changed the example app to list examples based on whether they support Google Maps on iOS. The logic was such that on Android it only showed the examples that are iOS + Google Map compatible, however on Android this condition is irrelevant, this commit changes it to show all examples on Android, regardless.
Setup
Screenshot