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

Support Google Maps on iOS #548

Merged
merged 8 commits into from
Sep 22, 2016
Merged

Support Google Maps on iOS #548

merged 8 commits into from
Sep 22, 2016

Conversation

gilbox
Copy link
Contributor

@gilbox gilbox commented Sep 7, 2016

Setup

# cd into example/
npm install

# cd into example/ios/
pod install
open AirMapsExplorer.xcworkspace

Screenshot

map

@spikebrehm
Copy link

Looks like you need to rebase.

@@ -33,3 +33,4 @@ local.properties
#
node_modules/
npm-debug.log
Pods/

Choose a reason for hiding this comment

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

add trailing newline

@jrichardlai
Copy link
Contributor

Wow this looks interesting :)! Thanks for making an early PR!

@fkoester
Copy link

The PR title kind of confused me. Maybe rename it to "Google Maps on iOS"? 😉

@gilbox gilbox changed the title GoogleMaps support GoogleMaps support for iOS Sep 14, 2016
@gilbox gilbox changed the title GoogleMaps support for iOS [WIP] GoogleMaps support for iOS Sep 14, 2016
@gilbox gilbox changed the title [WIP] GoogleMaps support for iOS GoogleMaps support for iOS Sep 15, 2016
@gilbox
Copy link
Contributor Author

gilbox commented Sep 15, 2016

Far from feature parity, but I think that this is ready.

@2ducanhpham
Copy link

when feature release? @gilbox

@lelandrichardson lelandrichardson changed the title GoogleMaps support for iOS Support Google Maps on iOS Sep 19, 2016
Copy link
Collaborator

@lelandrichardson lelandrichardson left a 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',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Typo?

Copy link
Contributor Author

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.

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,
Copy link
Collaborator

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,
Copy link
Collaborator

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?

Copy link
Collaborator

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') {
Copy link
Collaborator

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...

Copy link
Contributor Author

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
Copy link
Collaborator

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([
Copy link
Collaborator

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"];
Copy link
Collaborator

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?

Copy link
Contributor Author

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?

Choose a reason for hiding this comment

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

Copy link
Contributor Author

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

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 => [
Copy link
Collaborator

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];
Copy link
Collaborator

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?

Copy link
Contributor Author

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];
Copy link
Collaborator

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) {

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',

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 }) {

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 }) {

Copy link
Collaborator

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';

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;

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),

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;

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() {

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)'],

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>

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>?

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.

Copy link

@spikebrehm spikebrehm left a 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):

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?

Copy link
Contributor Author

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

Choose a reason for hiding this comment

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

TODO?

Copy link
Contributor Author

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)

Choose a reason for hiding this comment

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

not supported?

Copy link
Contributor Author

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;

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.

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 = {};
Copy link

@spikebrehm spikebrehm Sep 21, 2016

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;

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();
}

Copy link
Contributor Author

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',

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.

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';

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 = {};

Choose a reason for hiding this comment

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

This can be const

@spikebrehm
Copy link

Smoke tested on iOS, it works great!!!

@gilbox
Copy link
Contributor Author

gilbox commented Sep 21, 2016

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 */

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,

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.

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!!

@julien-rodrigues
Copy link
Contributor

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

@spikebrehm
Copy link

Wooo 🎉 🍻 let's do this!

@gilbox gilbox merged commit f78c0ee into master Sep 22, 2016
@gilbox gilbox deleted the gil/google branch September 22, 2016 17:03
spikebrehm pushed a commit that referenced this pull request Sep 22, 2016
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.
@julien-rodrigues
Copy link
Contributor

This was unexpected haha 🎉 🍻 congrats

timxyz pushed a commit to 3sidedcube/react-native-maps that referenced this pull request Sep 26, 2016
* 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
  ...
@stereodenis
Copy link
Contributor

@gilbox @spikebrehm is it possible to use it without cocoapods?

@rops
Copy link
Contributor

rops commented Oct 5, 2016

Has anyone noticed a performance regression on Android with this change? I'm still investigating, but it seems like calling animateToRegion on Android got slower with this commit.

@gilbox
Copy link
Contributor Author

gilbox commented Oct 6, 2016

@stereodenis unfortunately GoogleMaps SDK for iOS only supports install with cocoapods

kfiroo pushed a commit to kfiroo/react-native-maps that referenced this pull request Oct 13, 2016
kfiroo pushed a commit to kfiroo/react-native-maps that referenced this pull request Oct 13, 2016
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.
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