Fix zoom animation for v3.32 by stephenfarrar · Pull Request #559 · google-map-react/google-map-react

Fix zoom animation for v3.32 by stephenfarrar · Pull Request #559 · google-map-react/google-map-react

I managed to reduce the surface of the change.
The basic demo seems to work fine, but I haven't tried it in any other demos.

Summary of change:

This uses a better method to position the marker container div in the map top-left.
It also lets each marker use the fromLatLngToContainerPixel() method, which accounts for fractional zoom during a zoom animation.

Fixes #552

Instead, add a new method fromLatLngToContainerPixel() to geo service.

Choose a reason for hiding this comment

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

Great work overall! I've a couple of questions and comments, I am pretty excited to get this merged, let me know if you need any help!

		} else {
		this_.updateCounter_++;
		this_._onBoundsChanged(map, maps);
		if (mapsVersion < 32) {

Choose a reason for hiding this comment

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

Can we move 32 to a const variable?

Choose a reason for hiding this comment

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

Done.

		const sw = bounds.getSouthWest();
		const ptx = overlayProjection.fromLatLngToDivPixel(
		new maps.LatLng(ne.lat(), sw.lng())
		overlayProjection.fromContainerPixelToLatLng({ x: 0, y: 0 })

Choose a reason for hiding this comment

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

Could you elaborate why setting { x:0, y:0 } fixes the animation problem?

Choose a reason for hiding this comment

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

Good question, this is subtle. There are two ways of getting the lat-lng coordinates of the map's top-left corner.

Using map.getBounds() gives you the final bounds. That's because it is based on map.getCenter() and map.getZoom() which give you the final center and the final zoom. In other words, if you call map.setZoom() or click on the zoom control, map.getZoom() immediately reflects the new zoom, and so does map.getBounds(). You would use this if you were, say, fetching data from a server based on the map bounds.
Using overlayProjection.fromContainerPixelToLatLng({ x: 0, y: 0}) gives you the bounds for the current frame. We use this because we are drawing each frame.

Choose a reason for hiding this comment

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

@stephenfarrar Great, thanks for the answer. That makes sense, if map.getBounds gives you the final bounds, that is exactly what is making the markers animate from the corners.

		this._setLayers(this.props.layerTypes);

		const versionMatch = maps.version.match(/^3\.(\d+)\./);
		const mapsVersion = versionMatch ? Number(versionMatch[1]) : 99;

Choose a reason for hiding this comment

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

Could we make this code more readable? What is 99? or versionMatch[1]? (I understand ofc but its not so readable)

Choose a reason for hiding this comment

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

Done.

		const DEFAULT_MIN_ZOOM = 3;
		// Starting with version 3.32, the maps API calls `draw()` each frame during
		// a zoom animation.
		const DRAW_CALLED_DURING_ANIMATION = 32;

Choose a reason for hiding this comment

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

@stephenfarrar Good work, I will add _VERSION to this const real quick if you don't mind.

Choose a reason for hiding this comment

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

@stephenfarrar Although I keep thinking if we want to keep simulating the draw() for versions below 3.32, as this map uses the latest version as default 🤔. We could remove this whole block: https://github.com/google-map-react/google-map-react/pull/559/files#diff-0146a78a4269b3d2562b8f44d0137200R673

I understand that removing this would affect our support for older versions, but at the same time I don't believe we should have different code for each version of Google Map (I know its not the case and its just for versions below 3.32, but my question is more related to which approach to take when).

What do you think?

🤔

I wrote it this way because I think it's good for customers to be able to choose independently the version of the Maps API and google-map-react.

But I agree that this version-detection is not very good.

We can delete this section in late August 2018, because v3.31 is scheduled to be deleted by then. Does that sound acceptable?

adriamarzo added a commit to Badiapp/google-map-react that referenced this pull request

@stephenfarrar I think it does! Lets keep it as it is right now, I will delete it afterwards if needed.

naoak mentioned this pull request

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 bot locked as resolved and limited conversation to collaborators