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

Added heatmap functionality #441

Merged
merged 17 commits into from Feb 12, 2018
Merged

Conversation

ZAKdev
Copy link
Contributor

@ZAKdev ZAKdev commented Sep 10, 2017

Hi, I have added heatmap functionality which is from default google map api.

How it works
Just need to pass prop of heatmap with GoogleMapReact component and all heatmap options are available like radius, opacity and gradient.

Example

<GoogleMapComp
  lat={this.props.lat}
  lng={this.props.lng}
  zoom={this.props.zoom}
  products={this.props.products}
  heatmap={{
    positions: [{
        lat: 24.075,
        lng: 54.947,
      },
      {
        lat: 24.075,
        lng: 54.940,
      },
      {
        lat: 24.075,
        lng: 54.943,
      },
      {
        lat: 24.075,
        lng: 54.933,
      },
      {
        lat: 24.075,
        lng: 54.923,
      }],
      options: {
        radius: 20,
        opacity: 0.7,
        gradient: [
          'rgba(0, 255, 255, 0)',
          'rgba(0, 255, 255, 1)',
          'rgba(0, 191, 255, 1)',
          'rgba(0, 127, 255, 1)',
          'rgba(0, 63, 255, 1)',
          'rgba(0, 0, 255, 1)',
          'rgba(0, 0, 223, 1)',
          'rgba(0, 0, 191, 1)',
          'rgba(0, 0, 159, 1)',
          'rgba(0, 0, 127, 1)',
          'rgba(63, 0, 91, 1)',
          'rgba(127, 0, 63, 1)',
          'rgba(191, 0, 31, 1)',
          'rgba(255, 0, 0, 1)'
        ]
      }
    }
  }
</GoogleMapComp>

Copy link
Member

@itsmichaeldiego itsmichaeldiego left a comment

Choose a reason for hiding this comment

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

This is great, would you mind creating tests and adding an example?

@ZAKdev
Copy link
Contributor Author

ZAKdev commented Dec 21, 2017

No problem @itsmichaeldiego, i will write tests and add example asap.

Copy link
Member

@itsmichaeldiego itsmichaeldiego left a comment

Choose a reason for hiding this comment

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

Looking good generally, I left a couple of comments

@@ -0,0 +1,103 @@
/* eslint-disable */
Copy link
Member

Choose a reason for hiding this comment

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

Could we enable eslint here and fix the errors?

@@ -0,0 +1,103 @@
/* eslint-disable */
import React, { PropTypes } from 'react';
import compose from 'recompose/compose';
Copy link
Member

Choose a reason for hiding this comment

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

Lets put everything from recompose in one import as:

import {
  compose,
  defaultProps,
  withHandlers,
  withState,
  withContext,
  withProps,
  withPropsOnChange,
} from 'recompose';

import GoogleMapReact from '../src';
import SimpleMarker from './markers/SimpleMarker';
import { createSelector } from 'reselect';
import { susolvkaCoords, generateMarkers, heatmapData } from './data/fakeData';
Copy link
Member

Choose a reason for hiding this comment

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

Lets put first global imports and then local imports, meaning that we should have something like:

import React, { PropTypes } from 'react';
import { blablabla } from 'recompose';
import { createSelector } from 'reselect';

import GoogleMapReact from '../src';
import SimpleMarker from './markers/SimpleMarker';
import withStateSelector from './utils/withStateSelector';
import ptInBounds from './utils/ptInBounds';
import { susolvkaCoords, generateMarkers, heatmapData } from './data/fakeData';

@@ -10,6 +10,7 @@ import MarkerDispatcher from './marker_dispatcher';
import GoogleMapMap from './google_map_map';
import GoogleMapMarkers from './google_map_markers';
import GoogleMapMarkersPrerender from './google_map_markers_prerender';
import * as GoogleHeatmap from './google_heatmap';
Copy link
Member

Choose a reason for hiding this comment

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

Lets specify what we want to import, instead of importing everything, just to be sure.

@@ -63,7 +63,7 @@ export default function googleMapLoader(bootstrapURLKeys) {
);

$script_(
`https://maps.googleapis.com/maps/api/js?callback=_$_google_map_initialize_$_${queryString}`,
`https://maps.googleapis.com/maps/api/js?callback=_$_google_map_initialize_$_${queryString}&libraries=visualization`,
Copy link
Member

Choose a reason for hiding this comment

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

Could you add information of why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

visualization library includes the HeatmapLayer
Here is the reference https://developers.google.com/maps/documentation/javascript/visualization

Copy link
Member

Choose a reason for hiding this comment

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

@ZAKdev That is good, but shouldn't we add this optional if the user sets heatmap as true? Sorry if I am getting it wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@itsmichaeldiego, I can check heatmap prop if its not defined then no need to load visualization library, but this approach will work only if user has single map on a page because google map api load once for all maps so user may can face some problem.

Copy link
Member

Choose a reason for hiding this comment

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

@ZAKdev That is a good catch. I am wondering if adding visualization that includes the HeatmapLayer if we don't want Heatmap would make the performance worse 🤔

Copy link
Member

Choose a reason for hiding this comment

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

@ZAKdev Let me take a look, but what about if the user just set the library when using bootstrapURLKeys? This is called only once when the map is loaded as it contains the key for it, there you can set for example heatmap: true and it won't be initialized twice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@itsmichaeldiego, this is what i was doing before, but i am assuming a scenario where user has more then one map and any of the map has heatmap at this point it will throw exception because we are initializing map based on user first map.

If first map is the heatmap then every map will work smoothly.

Copy link
Member

Choose a reason for hiding this comment

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

@ZAKdev Oh right, so unless we find a workaround we could request the users to set heatmap: true to every map if they want at least one of the maps to have this functionality. But should we written in the docs for sure.

Otherwise we would have to force the user to share the same bootstrapURLKeys in all the maps, and that is something we wouldn't want to lose.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@itsmichaeldiego i will commit soon for this.

Copy link
Member

Choose a reason for hiding this comment

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

@ZAKdev You rock!

@ZAKdev
Copy link
Contributor Author

ZAKdev commented Jan 9, 2018

@itsmichaeldiego, i have added prop called heatmapLibrary it can be true and false for enable and disable visualization library.

if everything works fine for you so i can add prop heatmapLibrary={true} with every component.

@itsmichaeldiego
Copy link
Member

@ZAKdev Great work man! So the outcome is that we should pass heatmapLibrary={true} to all the maps if at least one of them is using heatmap, right?

In that case, adding that to the docs would be great, then I will merge this and we can make a new release!

@ZAKdev
Copy link
Contributor Author

ZAKdev commented Jan 18, 2018

@itsmichaeldiego i am going to add heatmapLibrary={true} for all our demo maps because we have demo for heatmap as well.

@itsmichaeldiego
Copy link
Member

@ZAKdev Awesome dude, let me know and I will merge and make a release! Keep in mind we've some little conflicts with the last PR I merged

@ZAKdev
Copy link
Contributor Author

ZAKdev commented Jan 18, 2018

@itsmichaeldiego, i found some issues in master

screen shot 2018-01-19 at 2 38 02 am

@itsmichaeldiego
Copy link
Member

@ZAKdev Good catch, that has already been fixed, do you mind pulling the latest version and checking again?

@ZAKdev
Copy link
Contributor Author

ZAKdev commented Jan 24, 2018

@itsmichaeldiego now we are good to go, you can add heatmapLibrary={true} in README

@itsmichaeldiego
Copy link
Member

@ZAKdev Awesome! Thanks!! Changes are looking good, would you mind including that in the README? So that PR handles all the work? LMK and I will merge it asap and publish a new version!

@ZAKdev
Copy link
Contributor Author

ZAKdev commented Jan 26, 2018

@itsmichaeldiego, i have added it in README with example code, please let me know if i needs to change something.

onChildMouseEnter={onChildMouseEnter}
onChildMouseLeave={onChildMouseLeave}
resetBoundsOnResize={true}
apiKey={'AIzaSyC-BebC7ChnHPzxQm7DAHYFMCqR5H3Jlps'}
Copy link
Member

Choose a reason for hiding this comment

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

@ZAKdev What is this apiKey here for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@itsmichaeldiego, we can remove it, its not doing anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@itsmichaeldiego, i removed.

Copy link
Member

Choose a reason for hiding this comment

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

@ZAKdev If I am not mistaken I believe there are a few more lines that could be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@itsmichaeldiego i removed unnecessary props

Copy link
Member

Choose a reason for hiding this comment

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

Great dude! Thanks

@calvintychan
Copy link

Hey guys, any chance we can merge this feature soon? :)

@itsmichaeldiego
Copy link
Member

itsmichaeldiego commented Feb 12, 2018

Yes @calvintychan I am merging it but we will have to test with your heatmaps

@itsmichaeldiego itsmichaeldiego merged commit e50f1d8 into google-map-react:master Feb 12, 2018
@itsmichaeldiego
Copy link
Member

@calvintychan @ZAKdev Merged, could you guys test it out with your heatmaps and let me know?

@ZAKdev
Copy link
Contributor Author

ZAKdev commented Feb 13, 2018

Thanks @itsmichaeldiego, I will check everything soon and update you here.

Hi @calvintychan, if you need any of guidance for using this feature please feel free to ask, i am available here for help.

@itsmichaeldiego
Copy link
Member

itsmichaeldiego commented Feb 14, 2018

Thanks @ZAKdev, looking forward to it

@ZAKdev
Copy link
Contributor Author

ZAKdev commented Feb 14, 2018

@itsmichaeldiego, Heatmap is working as expected 💯
Now lets wait for @calvintychan feedback.

@ribeiroguilherme
Copy link

@ZAKdev @itsmichaeldiego And what about support locations with weight? Is there any plans to implement it?
e.g.
var heatMapData = [ {location: new google.maps.LatLng(37.782, -122.447), weight: 0.5}, ....

@ZAKdev
Copy link
Contributor Author

ZAKdev commented Apr 11, 2018

@ribeiroguilherme, Good idea, i didn't really know we can define weight as well, i will add it soon :)

@lock
Copy link

lock bot commented Dec 1, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants