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

Make resetBoundsOnResize preserve center when full-screened #482

Merged
merged 9 commits into from Dec 22, 2017

Conversation

josephfrazier
Copy link
Contributor

This supersedes and resolves #308,
as specified in #308 (comment)

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.

Thanks for your PR! It is looking good, I left a comment that is more like a question

@@ -398,7 +399,7 @@ export default class GoogleMap extends Component {
window.removeEventListener('keydown', this._onKeyDownCapture);
mapDom.removeEventListener('mousedown', this._onMapMouseDownNative, true);
window.removeEventListener('mouseup', this._onChildMouseUp, false);
if (this.props.resetBoundsOnResize) {
if (this.props.resetBoundsOnResize || this.props.lockCenter) {
Copy link
Member

Choose a reason for hiding this comment

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

Wondering if || this.props.lockCenter is really needed, as resetBoundsOnResize should be set to true if we want to lock the center, not sure if I am explaining myself, but I believe the map always should re-set the center when reseting the bounds on resize.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I think I see what you're saying. I hadn't used resetBoundsOnResize before, but I just tried it out on the master branch and on this PR's branch by running yarn start and visiting http://localhost:4000/resizable. On both branches, resizing by dragging the lower- the map keeps the map centered, but only with this PR does clicking the fullscreen button also keep the map centered. I think it makes sense for resetBoundsOnResize to include the lockCenter behavior, so I removed the lockCenter stuff while keeping the behavioral changes. See 692e15b

@josephfrazier josephfrazier changed the title Add lockCenter prop to preserve center on resize Make resetBoundsOnResize preserve center when full-screened Dec 21, 2017
@itsmichaeldiego
Copy link
Member

itsmichaeldiego commented Dec 21, 2017

@josephfrazier Just to confirm, did you test latest changes? Could you upload something?

@josephfrazier
Copy link
Contributor Author

Yes, I verified the fix as outlined above, by running yarn start, then visiting http://localhost:4000/resizable and clicking the full-screen button.

@itsmichaeldiego itsmichaeldiego merged commit 0db8020 into google-map-react:master Dec 22, 2017
@josephfrazier josephfrazier deleted the lockCenter branch December 23, 2017 02:37
@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

3 participants