Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Added feature: update heat map on data change + fix linting#593

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

DonovanDeSmedt
Copy link
Contributor

When prop updateHeatmap is set to true --> map will rerender.

  • New render of and data is changed, prop updateHeatmap should be true
  • New render of and data is not changed, prop updateHeatmap should be false

@itsmichaeldiego
Copy link
Member

@DonovanDeSmedt Great work dude! Do you mind adding a quick video showing it works?

@ZAKdev as you were the one who added heatmap and you work often with it, do you mind taking a quick look?

@DonovanDeSmedt
Copy link
ContributorAuthor

DonovanDeSmedt commentedJun 5, 2018
edited
Loading

@itsmichaeldiego thx, was quiet busy last week, I'll make a video tomorrow.

@DonovanDeSmedt
Copy link
ContributorAuthor

A small example video of this feature:
https://drive.google.com/file/d/1at53N8L4BaktsP2ucipl6Rd_6P_lk92e/view?usp=sharing

Only negative point that could be improved is the visual rerender of the map (map becomes grey for a second before a rerender). It would improve the UX if the map would rerender smoothly, without a visual grey interval...

this._isCenterDefined(nextProps.center)
(!this._isCenterDefined(this.props.center) &&
this._isCenterDefined(nextProps.center)) ||
this.props.updateHeatmap

Choose a reason for hiding this comment

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

This is perfect, I am wondering if we could add this logic into another block to make it clearer, something like:

if (this.props.updateHeatmap) {  this.initialized_ = this.props.updateHeatmap ? false : this.initialized_;  setTimeout(() => this._initMap(), 0);}

That we we don't callthis.initialized_ = this.props.updateHeatmap ? false : this.initialized_; everytime we change the center. I know we repeat the codesetTimeout(() => this._initMap(), 0); but I don't see it that bad.

Copy link
Member

@itsmichaeldiegoitsmichaeldiegoJun 19, 2018
edited
Loading

Choose a reason for hiding this comment

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

Also, I am a bit worried, my dear friend@istarkov left a commenthere that map should be initialized only once, I am pretty sure there is a way to achieve updating the map without re-initializing it, I think that is the reason why themap becomes grey for a second before a rerender, this also will affect the performance of your app, would you like to work on this together to find the best way to do it?

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Yeah, that's what I thought. It's a quick 'fix' for the moment but there should be a better way to implement this. I'll try to look into it.

@itsmichaeldiego
Copy link
Member

@DonovanDeSmedt BTW, I just fixed the build and in order to do so I've to fix the linter, so you might have to rebase and remove the fix linting comment. Let me know if you don't have time and I can do it for you.

When prop updateHeatmap is set to true --> map will rerender.- New render of <GoogleMapReact> and data is changed, prop updateHeatmap should be true- New render of <GoogleMapReact> and data is not changed, prop updateHeatmap should be false
@DonovanDeSmedt
Copy link
ContributorAuthor

Feel free to do it for me, I messed up some things locally :(

@DonovanDeSmedtDonovanDeSmedtforce-pushed thefeature/update_heatmap_on_data_change branch from3fe4444 to6344453CompareOctober 21, 2018 18:17
@DonovanDeSmedt
Copy link
ContributorAuthor

Added support for extra google map api libraries:

  • Drawing library
  • Geometry library
  • Places library

Updated the rerender of heatmap when the prop updateHeatmap is set to true (no more flickering). Map is just being rerendered without being initialised again.

@itsmichaeldiego
Copy link
Member

@DonovanDeSmedt Great work man! I might need you to add it to the documentation so users know that this is a possibility! (To use this libraries). And I will create a minor release asap.

@DonovanDeSmedt
Copy link
ContributorAuthor

@itsmichaeldiego Indeed, that's necessary. I'll update the documentation in the readme.

@itsmichaeldiego
Copy link
Member

@DonovanDeSmedt Thanks! Looking forward for it!

- added info about the use of other libraries in the Google Map API- added info about the updateHeatmap prop
@DonovanDeSmedt
Copy link
ContributorAuthor

@itsmichaeldiego Can you check the updated readme to verify there is enough information about the change in this pr?

@itsmichaeldiego
Copy link
Member

@DonovanDeSmedt Looks great, thanks man! I will merge it soon

DonovanDeSmedt reacted with thumbs up emoji

@itsmichaeldiegoitsmichaeldiego merged commit1ce8726 intogoogle-map-react:masterNov 8, 2018
itsmichaeldiego added a commit that referenced this pull requestNov 8, 2018
This includes: -#655 -> Add math abs to avoid negative values when calculating zoom -#656 -> Pass map instance to onDrag handler -#593 -> Added feature: update heat map on data change + fix linting
@itsmichaeldiegoitsmichaeldiego mentioned this pull requestNov 8, 2018
itsmichaeldiego added a commit that referenced this pull requestNov 8, 2018
This includes: -#655 Add math abs to avoid negative values when calculating zoom -#656 Pass map instance to onDrag handler -#593 Added feature: update heat map on data change + fix linting
@itsmichaeldiego
Copy link
Member

@DonovanDeSmedt Merged and deployed in version1.1.0, please try it and let me know

@itsmichaeldiego
Copy link
Member

itsmichaeldiego commentedNov 13, 2018
edited
Loading

@DonovanDeSmedt This created an issue:#673

Let me know if you can take a look at that, because the older way of using libraries stopped working

Maybe we should change the approach to use heatmapLibrary the same way we used to add the libraries in the URL:

<GoogleMapReact  bootstrapURLKeys={{    key: 'xxxxxxxxxxxxxx',    libraries: ['places', 'drawing', 'heatmap']  }}  yesIWantToUseGoogleMapApiInternals  onGoogleApiLoaded={({ maps }) => {    console.log(maps.places);    console.log(maps.drawing);  }}/>

@DonovanDeSmedt
Copy link
ContributorAuthor

DonovanDeSmedt commentedNov 20, 2018
edited
Loading

@DonovanDeSmedt This created an issue:#673

Let me know if you can take a look at that, because the older way of using libraries stopped working

Maybe we should change the approach to use heatmapLibrary the same way we used to add the libraries in the URL:

<GoogleMapReact  bootstrapURLKeys={{    key: 'xxxxxxxxxxxxxx',    libraries: ['places', 'drawing', 'heatmap']  }}  yesIWantToUseGoogleMapApiInternals  onGoogleApiLoaded={({ maps }) => {    console.log(maps.places);    console.log(maps.drawing);  }}/>

Ok, I'll update it, by the weekend at the latest.
Do I offer both ways to use the libraries at the moment and deprecate the 'wrong' way in the next release?

Correct way:

<GoogleMapReact  bootstrapURLKeys={{    key: 'xxxxxxxxxxxxxx',    libraries: ['places', 'drawing', 'heatmap']  }}  yesIWantToUseGoogleMapApiInternals  onGoogleApiLoaded={({ maps }) => {    console.log(maps.places);    console.log(maps.drawing);  }}/>

Wrong way:

<GoogleMapReact  bootstrapURLKeys={{    key: 'xxxxxxxxxxxxxx',  }}  yesIWantToUseGoogleMapApiInternals  drawingLibrary={true}  placesLibrary ={true}  onGoogleApiLoaded={({ maps }) => {    console.log(maps.places);    console.log(maps.drawing);  }}/>

@itsmichaeldiego
Copy link
Member

@DonovanDeSmedt Lets just use the "correct way" :D

@snhasani
Copy link

drawingLibrary={true}placesLibrary ={true}

OMG! I can't believe you changed an important API without any notice, updating docs and release log. This is super critical and ppl are using this component in a serious business and product. Please bring back the wrong way, anyway.

itsmichaeldiego added a commit that referenced this pull requestNov 21, 2018
…679)* Revert "Bump version to 1.1.0 (#671)"This reverts commit1603e3a.* Revert "Added feature: update heat map on data change + fix linting (#593)"This reverts commit1ce8726.
DonovanDeSmedt added a commit to DonovanDeSmedt/google-map-react that referenced this pull requestNov 21, 2018
* 'master' of github.com:google-map-react/google-map-react:  Bump to 1.1.1 (google-map-react#680)  Revert "Added feature: update heat map on data change + fix linting" (google-map-react#679)  Bump version to 1.1.0 (google-map-react#671)  Added feature: update heat map on data change + fix linting (google-map-react#593)  Pass map instance to onDrag handler (google-map-react#656)  add math abs to avoid negative values when calculating zoom (google-map-react#655)  Bump version to 1.0.9 (google-map-react#651)  Custom div style options (google-map-react#634)  Bump version to 1.0.8 (google-map-react#646)  Revert 643 fix/map context (google-map-react#645)  Bump version to 1.0.7 (google-map-react#644)  Add passive scroll (google-map-react#631)  Use React 16 portal to render map overlay (google-map-react#643)  Fix old examples links and add one to new examples (google-map-react#633)  Bump version to 1.0.6 (google-map-react#621)  Add prop `onTilesLoaded` (google-map-react#615)  Fix typo, and call fromContainerPixelToLatLng() as you would expect. (google-map-react#620)  Update API.md (google-map-react#611)  Upgrade version to 1.0.5 (google-map-react#607)  Remove marker jiggle. (google-map-react#603)
@DonovanDeSmedt
Copy link
ContributorAuthor

The new implemenation of loading libraries was added to be consistent with the heatmap library (as described in theapi docs).

drawingLibrary={true}

This implementation no longer supported the previous way

bootstrapURLKeys={{    key: 'xxxxxxxxxxxxxx',    libraries: ['places', 'drawing', 'heatmap']  }}

It was released in release1.1.0 but it should indeed be backward compatible. Hence mypr to fix this issue (and support both ways) if that's wanted.

@itsmichaeldiego
Copy link
Member

Thanks@DonovanDeSmedt! Great input, I already answered in your PR, I think the heatmap change was created a few months back by@ZAKdev and I did not remember that we already had another way to handle libraries, that was my mistake. I would keep it the old way, just importing libraries with the array.

@itsmichaeldiego
Copy link
Member

itsmichaeldiego commentedNov 22, 2018
edited
Loading

@snhasani We fixed this and released1.1.1

snhasani reacted with thumbs up emoji

@lock
Copy link

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

@locklockbot locked asresolvedand limited conversation to collaboratorsDec 1, 2019
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@itsmichaeldiegoitsmichaeldiegoitsmichaeldiego left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

3 participants
@DonovanDeSmedt@itsmichaeldiego@snhasani

[8]ページ先頭

©2009-2025 Movatter.jp