tomchentw / react-google-maps

React.js Google Maps integration component
https://tomchentw.github.io/react-google-maps/
MIT License
4.62k stars 939 forks source link

Expose Resize Event on Google Maps #687

Open MGHawes opened 6 years ago

MGHawes commented 6 years ago

So I've been using this component for a while and ran into a known issue with the Google Maps Api v3. Namely this issue. The upshot being that when the component updates I need to trigger a resize event. Currently I'm doing this in the most horrible way imaginable:

export const ListingsMap = compose<WithScriptjsProps & WithGoogleMapProps, MapProps>(
  withProps({
    googleMapURL: `https://maps.googleapis.com/maps/api/js?key=${googleMapsApiKey}&v=3.exp&libraries=geometry,drawing,places`,
    loadingElement: <div style={{height: `100%`}}/>,
    containerElement: <div id="map" className="map"/>,
    mapElement: <div id="map" className="map" ref="map"/>,
  }),
  withScriptjs,
  withGoogleMaps,
  lifecycle({
    componentDidMount() {
      google.maps.event.trigger((window as any).googleMapsObject, 'resize');
    },
    componentDidUpdate() {
      google.maps.event.trigger((window as any).googleMapsObject, 'resize');
    },
    componentWillMount() {
      const refs: { map?: GoogleMap} = {};
      this.setState({
        onMapWillMount: ref => {
          refs.map = ref;
          (window as any).googleMapsObject = (refs.map as Component<any>).context['__SECRET_MAP_DO_NOT_USE_OR_YOU_WILL_BE_FIRED'];
        },
      });
    },
  }),
)((props) => (
  <GoogleMap
    ref={props.onMapWillMount}
    options={mapOptions}
  />
));

I've cut a lot out for brevity, essentially my issue is that I want to have the ability to resize as an exposed feature. Or via the map object itself exposed via some stable means; I'm open to suggestions. However, I think we should at least in principal expose as much of the Google Maps Api as possible/feasible?

I am more than happy to work on this an submit a PR but thought I'd get some guidance first about implementation. Hopefully I've been thorough enough and you guys think this is worth some time 😄

tomchentw commented 6 years ago

Yeah, I remove the event trigger in v8.0.0 rewrite.

Since we have a jscodeshift API, I'm thinking we can just add event trigger function on the component itself:

refs.map.trigger('resize');

// in GoogleMap.js
trigger(...args) {
  return google.maps.event.trigger(this.context[MAP], ...args);
}

What do you think?

MGHawes commented 6 years ago

That seems pretty clean to me and would suit my needs. I can't think of a reason why this would be an unsuitable interface for event.trigger(). My only thought would be that if we expose trigger() would it not also be sensible to expose the rest of the event namespace in some way. And if the whole namespace is exposed maybe there's a better way of doing it?

What are the objections to exposing the map object itself? This would eliminated the need to expose all parts of the Google maps api that interact with the map. I can see that it could lead to messy implementations where react-google-maps users try to overwrite behaviors of the GoogleMap class by using the map object. But really this comes down to the user making a poor choice? I'm open to refusal on this 😄, especially from a learning point of view.

tomchentw commented 6 years ago

the rest of the event namespace

It's already exposed as React props

What are the objections to exposing the map object itself?

google.maps.MVCObject are often used in a imperative way, which is in the opposite of what React does. I just want to make it simple, declarative and easy to use. For complex features, I can't find any good way to bridge it into React.

tomchentw commented 6 years ago

PRs welcomed. Help me please

StefanoPastore commented 6 years ago

@tomchentw can i make a PR with your solution:

refs.map.trigger('resize');

// in GoogleMap.js
trigger(...args) {
  return google.maps.event.trigger(this.context[MAP], ...args);
}
ninjasun commented 6 years ago

is there a working example of how to use it?

dmitru commented 5 years ago

Any updates? I'd LOVE to see that PR merged.