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

Fix zoom when map container is scaled #5794

Merged
merged 2 commits into from
Sep 29, 2017

Conversation

cherniavskii
Copy link
Collaborator

@cherniavskii cherniavskii commented Sep 21, 2017

Closes #5386, since that solution didn't work.
Fixes #2795.

@mourner mentioned, that reading transform can impact performance. So I'm calculating scale instead of reading it from stylesheet. I believe it will not impact performance. But still I would like to add memoization for scale calculation, since it rather wouldn't change often, and getMousePosition is fired every mousemove.

I'll also add a debug page with scaled map later.

@cherniavskii cherniavskii changed the title fix zoom when container is scaled Fix zoom when map container is scaled Sep 21, 2017
@mourner
Copy link
Member

mourner commented Sep 29, 2017

Looks good to me!

P.S. Seeing a lot of well-thought out PRs from you recently, I thought it's time to invite you to the contributor team in Leaflet. Hopefully this will make working on Leaflet a bit easier — now you can work directly in this repo in separate branches, and it will be easy for others to add commits to your branches when working on something. You now also have the right to manage/close issues. Welcome!

@mourner mourner merged commit 899bdd4 into Leaflet:master Sep 29, 2017
@cherniavskii
Copy link
Collaborator Author

@mourner thank you! ;)

@cherniavskii cherniavskii deleted the zoom-scaled-container branch October 1, 2017 07:55
@yarsky-tgz
Copy link

Sorry guys, but that fix really fixes nothing. Your commit means that we have all screen scaled, not the container. So if we have on page not only map (and map is scaled) - all is getting even worse then before.

I've fixed that by my own, because waiting fix of that for a long time... and actually have it already ). Thank you very much for great idea of how to fix it.

@cherniavskii
Copy link
Collaborator Author

@yarsky-tgz please submit an issue with example which reproduces that bug - it will really help to move your PR forward

@cherniavskii
Copy link
Collaborator Author

cherniavskii commented Jan 13, 2018

@yarsky-tgz looks like we don't have latest master branch hosted (see #5999), so currently there's no way to see how it works on current master without running project locally (at least until 1.3.0 release #5988).

I'd suggest you to build Leaflet locally and open debug/map-scaled.html debug page to check whether your issue is solved or not

@yarsky-tgz
Copy link

yarsky-tgz commented Jan 14, 2018

I've made demo site on heroku and commented #5997 (comment) .

Is that enough, or it shall be better to create issue too?

@cherniavskii
Copy link
Collaborator Author

cherniavskii commented Jan 14, 2018

@yarsky-tgz yes, please create separate isssues for each bug - in your case it's zoom issue and drag issue

@yarsky-tgz
Copy link

@cherniavskii Done.
#6001
#6002

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants