Skip to content

ZoomToLayer fix #635

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

Merged
merged 3 commits into from
Dec 16, 2021
Merged

ZoomToLayer fix #635

merged 3 commits into from
Dec 16, 2021

Conversation

ahmadayubi
Copy link
Member

@ahmadayubi ahmadayubi commented Dec 14, 2021

Rather than zooming in/out and checking if a layer is within bounds repeatedly, calculate whether a layer would be in or out of bounds after a set number of zoom in/outs, ahead of time.

Before: (flickering, instant movements)

2021-12-14.12-47-01.mp4

After: (no flickering, removing repeated zoom in/out)

2021-12-14.15-15-33.mp4

Closes issue with templatedFeatureLayer duplicates, although that issue may still remain in another form if the underlying bug wasn't the zoomToLayer feature but rather quick successive addition and removal of layers (which the old zoomToLayer feature would do).

Closes #634

#612 (comment)

@prushforth
Copy link
Member

It does seem to fix the problem with duplicate markers. The animation is not quite right though: it flashes dramatically. To reproduce/ see what I mean, go to this example. Click on British Columbia. Click on the marker. Use Zoom To Layer for the BC layer.

@ahmadayubi
Copy link
Member Author

ahmadayubi commented Dec 14, 2021

The flashing in that certain case is due to the tiles loading in, if you zoom out manually you can see the same effect, the animation is handled by leaflets flyTo function, alternatively you can simply setView to avoid animations altogether.

Removing the flying effect would also reduce bandwidth, as you wouldn't need to make requests for intermediary zoom level tiles. (the duplication and flickering would still be fixed)

@prushforth
Copy link
Member

Let's use the direct to location method then

@prushforth
Copy link
Member

Pretty abrupt, but functional. Can't afford to get too fancy atm. Makes me think that zooming to a feature is just another word for following a link.

@ahmadayubi
Copy link
Member Author

Maybe a fading effect would help, it would have to be a custom implementation since Leaflet doesn't offer layer fading in, I believe.

@prushforth
Copy link
Member

I don't think it makes much difference to non-visual users, so not inclined to spend cycles on it now.

@ahmadayubi ahmadayubi marked this pull request as ready for review December 16, 2021 16:50
Copy link
Member

@prushforth prushforth left a comment

Choose a reason for hiding this comment

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

LGTM -- thanks!

@prushforth prushforth merged commit 9f1d807 into Maps4HTML:main Dec 16, 2021
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.

zoomToLayer on TemplatedFeatureLayer causes feature duplication
3 participants