- Notifications
You must be signed in to change notification settings - Fork855
Fix zoom animation for v3.32#559
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Instead, add a new method fromLatLngToContainerPixel() to geo service.
There was a problem hiding this comment.
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!
src/google_map.js Outdated
| } else { | ||
| this_.updateCounter_++; | ||
| this_._onBoundsChanged(map, maps); | ||
| if (mapsVersion < 32) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
| const ptx = overlayProjection.fromLatLngToDivPixel( | ||
| new maps.LatLng(ne.lat(), sw.lng()) | ||
| overlayProjection.fromContainerPixelToLatLng({ x: 0, y: 0 }) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 thefinal bounds. That's because it is based onmap.getCenter()andmap.getZoom()which give you the final center and the final zoom. In other words, if you callmap.setZoom()or click on the zoom control,map.getZoom()immediately reflects the new zoom, and so doesmap.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 forthe current frame. We use this because we are drawing each frame.
There was a problem hiding this comment.
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, ifmap.getBounds gives you thefinal bounds, that is exactly what is making the markers animate from the corners.
src/google_map.js Outdated
| @@ -566,6 +566,9 @@ export default class GoogleMap extends Component { | |||
| this._setLayers(this.props.layerTypes); | |||
| const versionMatch = maps.version.match(/^3\.(\d+)\./); | |||
| const mapsVersion = versionMatch ? Number(versionMatch[1]) : 99; | |||
There was a problem hiding this comment.
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 is99? orversionMatch[1]? (I understand ofc but its not so readable)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Done.
src/google_map.js Outdated
| @@ -34,6 +34,9 @@ const K_GOOGLE_TILE_SIZE = 256; | |||
| const K_IDLE_TIMEOUT = 100; | |||
| const K_IDLE_CLICK_TIMEOUT = 300; | |||
| 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; | |||
There was a problem hiding this comment.
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.
itsmichaeldiego left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@stephenfarrar Great work, I think this is good to go.
itsmichaeldiego commentedApr 24, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@stephenfarrar Although I keep thinking if we want to keep simulating the 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? |
@stephenfarrar I think it does! Lets keep it as it is right now, I will delete it afterwards if needed. |
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. |
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:
fromLatLngToContainerPixel()method, which accounts for fractional zoom during a zoom animation.Fixes#552