Skip to content

Add prop onTilesLoaded#615

Merged
itsmichaeldiego merged 12 commits intogoogle-map-react:masterfrom
jonathanweiss:master
Aug 3, 2018
Merged

Add prop onTilesLoaded#615
itsmichaeldiego merged 12 commits intogoogle-map-react:masterfrom
jonathanweiss:master

Conversation

@jonathanweiss
Copy link
Contributor

There are some events that are fired by the Google Maps JavaScript API, but there are not available on the React component. With this PR, I'm passing through the event tilesloaded.

Please let me know what you think about this. I'd love to see my changes upstream.

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.

Good work, left a couple of comments

package.json Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Lets remove the @jfw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for that. I needed to publish the component in order to use it in a project. I'll remove it on master.

package.json Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Lets not make a version from this PR, version will be bumped when necessary with one or multiple PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same here.

Directly access the maps API - *use at your own risk!*

#### onTilesLoaded (func)

Copy link
Member

Choose a reason for hiding this comment

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

What about the info?

@jonathanweiss
Copy link
Contributor Author

I've applied all changes, please have another look @itsmichaeldiego.

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.

Great work man! I just left a very minor comment, looking forward to merge this asap.

CHANGELOG.md Outdated
Copy link
Member

Choose a reason for hiding this comment

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

@jonathanweiss Can we move this section to the bottom? And also, no need of add Unreleased as I am going to release it very soon after merged.

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 thing, I've removed that header. Not sure if I understand you correctly: you want me to move the entries for unreleased things to the bottom of the change log file?

@itsmichaeldiego
Copy link
Member

Can you delete the latest commit? Only the maintainers set the version releases

Copy link
Member

Choose a reason for hiding this comment

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

Should we add the listener all the times, or only when the user has set an onTilesLoaded prop? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At first I thought that this is just a micro optimisation, but this event is triggered a lot when the user is changing the viewport of the map. So it makes sense not to listen to the event if we don't need to. 👍

@itsmichaeldiego
Copy link
Member

@jonathanweiss Changes are looking great now! Thank you, I left a question for you before we merge this.

@jonathanweiss
Copy link
Contributor Author

Now we don't need _onTilesLoaded() any longer. 😉 Do you want to keep it for consistency?

@itsmichaeldiego
Copy link
Member

itsmichaeldiego commented Jul 31, 2018

@jonathanweiss What do you mean by that we don't need it any longer?

@jonathanweiss
Copy link
Contributor Author

Well, all that it does is checking if this.props.onTilesLoaded is truthy before calling it. Since we're now only attaching the listener when that prop is truthy, we could directly call this.props.onTilesLoaded(); in line 667.

@itsmichaeldiego
Copy link
Member

@jonathanweiss This is fine like this, thanks though!

@itsmichaeldiego itsmichaeldiego merged commit 6d142cb into google-map-react:master Aug 3, 2018
@jsharland
Copy link

Can someone please update the typescript definition to match as I could really do with the onTilesLoaded event.

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/google-map-react/index.d.ts

@vertic4l
Copy link

vertic4l commented Sep 5, 2018

@jsharland Type Definition are now up2date.
@jonathanweiss @itsmichaeldiego thank you!

DonovanDeSmedt added a commit to DonovanDeSmedt/google-map-react that referenced this pull request Nov 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)
@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.

4 participants

Comments