-
Notifications
You must be signed in to change notification settings - Fork 330
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 skipped map pan/zoom animation when switching location #2185
Conversation
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.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! all files reviewed, all discussions resolved
gui/src/renderer/components/SvgMap.tsx, line 289 at r1 (raw file):
const transform = useMemo(() => { const [x, y] = projection(center) ?? [0, 0]; return `translate(${width / 2 - x * zoom} ${height / 2 - y * zoom}) scale(${zoom})`;
FYI I am no big fan of math within string interpolation. I think it makes code harder to follow. I think it would be better to move those computations to variables.
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.
Reviewable status:
complete! all files reviewed, all discussions resolved
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.
Reviewable status:
complete! all files reviewed, all discussions resolved
e1a42cd
to
b1f9984
Compare
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.
Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @pronebird)
gui/src/renderer/components/SvgMap.tsx, line 289 at r1 (raw file):
Previously, pronebird (Andrej Mihajlov) wrote…
FYI I am no big fan of math within string interpolation. I think it makes code harder to follow. I think it would be better to move those computations to variables.
Yeah, I agree. Done.
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.
Reviewed 1 of 1 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
b1f9984
to
6538458
Compare
This PR adds a custom
ZoomableGroup
component which doesn't contain the issue where the map zooms and pans to the initial location. As a result the other workaround can be removed and therefore both the initial transition issue and the missing transition issue caused by the first workaround is fixed.The
ZoomableGroup
implementation can be removed when this issue has been solved:zcreativelabs/react-simple-maps#228
Git checklist:
CHANGELOG.md
under the[Unreleased]
header.This change is![Reviewable](https://camo.githubusercontent.com/23b05f5fb48215c989e92cc44cf6512512d083132bd3daf689867c8d9d386888/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)