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

.bounce() always uses original position #39

Closed
ulken opened this issue Sep 14, 2021 · 12 comments · Fixed by #42
Closed

.bounce() always uses original position #39

ulken opened this issue Sep 14, 2021 · 12 comments · Fixed by #42

Comments

@ulken
Copy link

ulken commented Sep 14, 2021

If the marker is moved after being added, .bounce() will move it back to its original position.

I would expect it to bounce at its current position.

I haven't scrutinized the code, but should be quite easy to listen to the move event and update the position accordingly?

@ulken
Copy link
Author

ulken commented Sep 15, 2021

@maximeh thoughts on the above?

I could look into submitting a PR, but just want to make sure you're onboard and/or if you see any pitfalls.

@ulken
Copy link
Author

ulken commented Sep 17, 2021

@maximeh I realize you have other things to tend to, but if you could just give a short view on the matter, that'd be appreciated.

Mainly after if this is a dead end or not.

@ulken
Copy link
Author

ulken commented Sep 17, 2021

Haven't tried it yet, but looking at the code I'm thinking it could be as easy as moving lines 172-173:

// Keep original latitude and longitude
this._origLatlng = this.getLatLng();

To lines 139-140, right before:

// Keep original map center
this._origMapCenter = this._map.project(this._map.getCenter());

That way the position will be up-to-date every time bounce() is used. Not only on when called from onAdd().

Thoughts?

@maximeh
Copy link
Owner

maximeh commented Sep 17, 2021

Heya, yeah I usually look at theses in the week end, I don't really have time during the week. I think what you're saying make sense and is worth a try yeah.
Do try if you zoom in/out while it's moving too as point should land in the right place at the end.

@ulken
Copy link
Author

ulken commented Sep 17, 2021

OK! Good to know 👍

I'll give it a try and put it through thorough testing.

Thank you for getting back to me 🙂

@ulken
Copy link
Author

ulken commented Sep 18, 2021

@maximeh another one ready for review. Both PRs are quite small changes. And for this one I'd say it's quite easy to reason that if the behavior was acceptable for onAdd(), this should be equally reasonable.

@maximeh
Copy link
Owner

maximeh commented Sep 19, 2021

Thanks a lot for taking the time to test and put out the PR; I'll test on my end, but it looks sane. As you said, it doesn't handle zooming really properly; moving in pixels would make sense, but it may also feel weird as the marker would bounce not where it's suppose to, and at the end of the animation, teleport where it should be.
I don't really see a good solution there and so far, there wasn't a demand to handle that case.

@ulken
Copy link
Author

ulken commented Sep 19, 2021

@maximeh Possible to release a new tag?

@ulken
Copy link
Author

ulken commented Sep 20, 2021

Or do you want it to be battle-tested for a while before putting a tag on it?

@ulken
Copy link
Author

ulken commented Sep 20, 2021

As you said, it doesn't handle zooming really properly; moving in pixels would make sense, but it may also feel weird as the marker would bounce not where it's suppose to, and at the end of the animation, teleport where it should be.

I don't really see a good solution there and so far, there wasn't a demand to handle that case.

Not super easy to test, but both Google and Apple Maps seem to handle this. I.e. zooming doesn't affect the animation. Not sure how they do it, though.

@maximeh
Copy link
Owner

maximeh commented Sep 20, 2021

@maximeh Possible to release a new tag?

Sure! I'll do that shortly, tags are cheap, no need to wait.

As you said, it doesn't handle zooming really properly; moving in pixels would make sense, but it may also feel weird as the marker would bounce not where it's suppose to, and at the end of the animation, teleport where it should be.
I don't really see a good solution there and so far, there wasn't a demand to handle that case.

Not super easy to test, but both Google and Apple Maps seem to handle this. I.e. zooming doesn't affect the animation. Not sure how they do it, though.

Hm interesting... That'd be worth checking, it would for sure be nicer. Is that something you'd be interesting in doing? I'll create an issue so it's there at least.

@ulken
Copy link
Author

ulken commented Sep 20, 2021

Hm interesting... That'd be worth checking, it would for sure be nicer. Is that something you'd be interesting in doing? I'll create an issue so it's there at least.

I'll probably do some digging/debugging at some point. Good idea to track it with an issue 👍

It might be that it's easier to achieve on a vector based map (which I think both Google and Apple use nowadays).

I also chatted briefly with a former Leaflet core-contributor (whom I happen to know). He said that there's quite a lot going on around marker positioning related to zooming. It might be that the code snippets are not working hand-in-hand and/or values are not up-to-date during zooming.

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 a pull request may close this issue.

2 participants