Skip to content

Commit

Permalink
feat(GoogleMapLoader): introduce loader to manage React elements
Browse files Browse the repository at this point in the history 
* Closes #141
* Closes #133

BREAKING CHANGE: GoogleMap with props.containerProps is now deprecated. Use GoogleMapLoader with props.googleMapElement instead

We also suggest switching to callback based ref so that you'll get the component instance when it is mounted.

Before:

```js
<GoogleMap containerProps={{
    ...this.props,
    style: {
      height: "100%",
    },
  }}
  ref="map"
  defaultZoom={3}
  defaultCenter={{lat: -25.363882, lng: 131.044922}}
  onClick={::this.handleMapClick}>
  {this.state.markers.map((marker, index) => {
    return (
      <Marker
        {...marker}
        onRightclick={this.handleMarkerRightclick.bind(this, index)} />
    );
  })}
</GoogleMap>
```

After:

```js
<GoogleMapLoader
  containerElement={
    <div
      {...this.props}
      style={{
        height: "100%",
      }}
    />
  }
  googleMapElement={
    <GoogleMap
      ref={(map) => console.log(map)}
      defaultZoom={3}
      defaultCenter={{lat: -25.363882, lng: 131.044922}}
      onClick={::this.handleMapClick}>
      {this.state.markers.map((marker, index) => {
        return (
          <Marker
            {...marker}
            onRightclick={this.handleMarkerRightclick.bind(this, index)} />
        );
      })}
    </GoogleMap>
  }
/>
```
  • Loading branch information
@tomchentw
tomchentw committed Nov 22, 2015
1 parent e21d803 commit 532816a
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 54 deletions.
108 changes: 55 additions & 53 deletions src/GoogleMap.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,8 @@ import {
} from "react";

import {
findDOMNode,
} from "react-dom";
default as warning,
} from "warning";

import {
default as GoogleMapHolder,
Expand All @@ -15,40 +15,47 @@ import {
mapEventPropTypes,
} from "./creators/GoogleMapHolder";

import {
default as GoogleMapLoader,
} from "./GoogleMapLoader";

const USE_NEW_BEHAVIOR_TAG_NAME = `__new_behavior__`;

export default class GoogleMap extends Component {
static propTypes = {
containerTagName: PropTypes.string.isRequired,
containerProps: PropTypes.object.isRequired,
containerTagName: PropTypes.string,
containerProps: PropTypes.object,
map: PropTypes.object,
// Uncontrolled default[props] - used only in componentDidMount
...mapDefaultPropTypes,
// Controlled [props] - used in componentDidMount/componentDidUpdate
...mapControlledPropTypes,
// Event [onEventName]
...mapEventPropTypes,
}
};

// Public APIs
//
// https://developers.google.com/maps/documentation/javascript/3.exp/reference#Map
//
// [].map.call($0.querySelectorAll("tr>td>code"), function(it){ return it.textContent; }).filter(function(it){ return it.match(/^get/) && !it.match(/Map$/); })
getBounds () { return this.state.map.getBounds(); }
getBounds () { return (this.props.map || this.refs.delegate).getBounds(); }

This comment has been minimized.

Sign in to view

Copy link
@maciejmyslinski

maciejmyslinski Dec 8, 2015

Just curious how You achived autobinding here... Could you explain, please?

This comment has been minimized.

Sign in to view

Copy link
@tomchentw

tomchentw Dec 9, 2015

Author Owner

What do you mean for autobinding? Here what I'm trying to do is:

  1. If the props.map exists, that means we're using new interface with <GoogleMapLoader>. So the props.map is our google.maps.Map instance, we can delegate the calls
  2. If that's not the case, assume we're in legacy code. Then we could delegate the calls to underlying <GoogleMap> component with refs.delegate

This comment has been minimized.

Sign in to view

Copy link
@maciejmyslinski

maciejmyslinski Dec 9, 2015

This is what I mean. You use Component class and do not bind this to methods anywere. How you achive that?

This comment has been minimized.

Sign in to view

Copy link
@tomchentw

tomchentw Dec 10, 2015

Author Owner

autobinding allows the method to be used with implicit this. As a result, you can invoke function directly without specifying the instance. Assume a instance method click on MyButton component:

var myBtn = this.refs.myBtn;// get the instance out
// oridinary calls
myBtn.click(); // invoke function with `this === myBtn`

var clickFn = myBtn.click; // get the function reference
// with autobinding:
clickFn(); // this === myBtn
// WITHOUT autobinding:
clickFn(); // this === undefined in strict mode

This comment has been minimized.

Sign in to view

Copy link
@tomchentw

tomchentw Dec 10, 2015

Author Owner

For your question, I don't do any autobinding here. These are class instance methods and they should have this when called. It's the caller's responsibility to bind this instead of GoogleMap component.

getCenter () { return this.state.map.getCenter(); }
getCenter () { return (this.props.map || this.refs.delegate).getCenter(); }

getDiv () { return this.state.map.getDiv(); }
getDiv () { return (this.props.map || this.refs.delegate).getDiv(); }

getHeading () { return this.state.map.getHeading(); }
getHeading () { return (this.props.map || this.refs.delegate).getHeading(); }

getMapTypeId () { return this.state.map.getMapTypeId(); }
getMapTypeId () { return (this.props.map || this.refs.delegate).getMapTypeId(); }

getProjection () { return this.state.map.getProjection(); }
getProjection () { return (this.props.map || this.refs.delegate).getProjection(); }

getStreetView () { return this.state.map.getStreetView(); }
getStreetView () { return (this.props.map || this.refs.delegate).getStreetView(); }

getTilt () { return this.state.map.getTilt(); }
getTilt () { return (this.props.map || this.refs.delegate).getTilt(); }

getZoom () { return this.state.map.getZoom(); }
getZoom () { return (this.props.map || this.refs.delegate).getZoom(); }
// END - Public APIs
//
// https://developers.google.com/maps/documentation/javascript/3.exp/reference#Map
Expand All @@ -59,55 +66,50 @@ export default class GoogleMap extends Component {
// https://developers.google.com/maps/documentation/javascript/3.exp/reference#Map
//
// [].map.call($0.querySelectorAll("tr>td>code"), function(it){ return it.textContent; }).filter(function(it){ return !it.match(/^get/) && !it.match(/^set/) && !it.match(/Map$/); })
fitBounds (bounds) { return this.state.map.fitBounds(bounds); }
fitBounds (bounds) { return (this.props.map || this.refs.delegate).fitBounds(bounds); }

panBy (x, y) { return this.state.map.panBy(x, y); }
panBy (x, y) { return (this.props.map || this.refs.delegate).panBy(x, y); }

panTo (latLng) { return this.state.map.panTo(latLng); }
panTo (latLng) { return (this.props.map || this.refs.delegate).panTo(latLng); }

panToBounds (latLngBounds) { return this.state.map.panToBounds(latLngBounds); }
panToBounds (latLngBounds) { return (this.props.map || this.refs.delegate).panToBounds(latLngBounds); }
// END - Public APIs - Use this carefully
//
// https://developers.google.com/maps/documentation/javascript/3.exp/reference#Map

static defaultProps = {
containerTagName: "div",
containerProps: {},
}

state = {
}
componentWillMount () {
const {containerTagName} = this.props;
const isUsingNewBehavior = USE_NEW_BEHAVIOR_TAG_NAME === containerTagName;

componentDidMount () {
const domEl = findDOMNode(this);
const {containerTagName, containerProps, children, ...mapProps} = this.props;
// TODO: support asynchronous load of google.maps API at this level.
//
// Create google.maps.Map instance so that dom is initialized before
// React's children creators.
//
const map = GoogleMapHolder._createMap(domEl, mapProps);
this.setState({ map });
warning(isUsingNewBehavior,
`"GoogleMap" with containerTagName is deprecated now and will be removed in next major release (5.0.0).
Use "GoogleMapLoader" instead. See https://github.com/tomchentw/react-google-maps/pull/157 for more details.`
);
}

render () {
const {containerTagName, containerProps, children, ...mapProps} = this.props;
const child = this.state.map ? (
// Notice: implementation details
//
// In this state, the DOM of google.maps.Map is already initialized in
// my innerHTML. Adding extra React components will not clean it
// in current version*. It will use prepend to add DOM of
// GoogleMapHolder and become a sibling of the DOM of google.maps.Map
// Not sure this is subject to change
//
// *current version: 0.13.3, 0.14.2
//
<GoogleMapHolder map={this.state.map} {...mapProps}>
{children}
</GoogleMapHolder>
) : undefined;

return React.createElement(containerTagName, containerProps, child);
const {containerTagName, containerProps = {}, children, ...mapProps} = this.props;
const isUsingNewBehavior = USE_NEW_BEHAVIOR_TAG_NAME === containerTagName;

if (isUsingNewBehavior) {
return (
<GoogleMapHolder {...mapProps}>
{children}
</GoogleMapHolder>
);
} else {//------------ Deprecated ------------
const realContainerTagName = null == containerTagName ? `div` : containerTagName;

return (
<GoogleMapLoader
containerElement={React.createElement(realContainerTagName, containerProps)}
googleMapElement={
<GoogleMap ref="delegate" containerTagName={USE_NEW_BEHAVIOR_TAG_NAME} {...mapProps}>
{children}
</GoogleMap>
}
/>
);
}
}
}
69 changes: 69 additions & 0 deletions src/GoogleMapLoader.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
import {
default as React,
PropTypes,
Component,
} from "react";

import {
default as propTypesElementOfType,
} from "react-prop-types-element-of-type";

import {
default as GoogleMapHolder,
} from "./creators/GoogleMapHolder";

const USE_NEW_BEHAVIOR_TAG_NAME = `__new_behavior__`;/* CIRCULAR_DEPENDENCY */

export default class GoogleMapLoader extends Component {
static propTypes = {
containerElement: PropTypes.node.isRequired,

This comment has been minimized.

Sign in to view

Copy link
@maciejmyslinski

maciejmyslinski Dec 2, 2015

Is there a need for .isRequired when down below we have containerElement in defaultProps?

This comment has been minimized.

Sign in to view

Copy link
@tomchentw

tomchentw Dec 3, 2015

Author Owner

Not sure. I may want to remove defaultProps in the future?
googleMapElement: PropTypes.element.isRequired,/* CIRCULAR_DEPENDENCY. Uncomment when 5.0.0 comes: propTypesElementOfType(GoogleMap).isRequired, */
};

static defaultProps = {
containerElement: (<div />),
};

state = {
map: null,
};

mountGoogleMap (domEl) {
if (this.state.map) {
return;
}
const {children, ...mapProps} = this.props.googleMapElement.props;
//
// Create google.maps.Map instance so that dom is initialized before
// React's children creators.
//
const map = GoogleMapHolder._createMap(domEl, mapProps);
this.setState({ map });
}

renderChild () {
if (this.state.map) {
// Notice: implementation details
//
// In this state, the DOM of google.maps.Map is already initialized in
// my innerHTML. Adding extra React components will not clean it
// in current version*. It will use prepend to add DOM of
// GoogleMapHolder and become a sibling of the DOM of google.maps.Map
// Not sure this is subject to change
//
// *current version: 0.13.3, 0.14.2
//
return React.cloneElement(this.props.googleMapElement, {
map: this.state.map,
//------------ Deprecated ------------
containerTagName: USE_NEW_BEHAVIOR_TAG_NAME,
});
}
}

render () {
return React.cloneElement(this.props.containerElement, {
ref: ::this.mountGoogleMap,
}, this.renderChild());
}
}
3 changes: 2 additions & 1 deletion src/index.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
export {default as GoogleMap} from "./GoogleMap";
export {default as GoogleMapLoader} from "./GoogleMapLoader";

export {default as GoogleMap} from "./GoogleMap";
export {default as Circle} from "./Circle";
export {default as DirectionsRenderer} from "./DirectionsRenderer";
export {default as DrawingManager} from "./DrawingManager";
Expand Down

3 comments on commit 532816a

@maciejmyslinski
Copy link

Choose a reason for hiding this comment

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

Can you explain why this change?
I'm learning a lot, from your code, you are good programmer.
BIG thank you for that library! 👍

Edit
I've seen issues that this commit closes, thank You one more time! 😃

@tomchentw
Copy link
Owner Author

Choose a reason for hiding this comment

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

Why for this change?

This PR decouples the creation logic of google.maps.Map instance and the mounting logic for <GoogleMap> component. As a result, we'll have:

The correct lifecycle for <GoogleMap> component

Because when it's mounted (ref callback invoked with the component instance), it's ready for use: invoking public APIs, reading values with getters ...etc

Consistent API with ScriptjsLoader

See more in the example below.

@tomchentw
Copy link
Owner Author

Choose a reason for hiding this comment

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

@maciejmyslinski thanks for the feedback!

Please sign in to comment.