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

feat: track map center and zoom in url, initial load map url params #1114

Closed
wants to merge 3 commits into from

Conversation

clintonlunn
Copy link
Collaborator

fixes #1113


name: Pull request
about: Create a pull request
title: ''
labels: ''
assignees: ''


What type of PR is this?(check all applicable)

  • refactor
  • feature
  • bug fix
  • documentation
  • optimization
  • other

Description

This pull request addresses #1113, where we want to create a shareable link that we can communicate zoom level and lat long center values.

In addition, added a skeleton to prevent the map from loading before the initial view state is available.

Related Issues

Issue #1113

What this PR achieves

Screenshots, recordings

Notes

Copy link

vercel bot commented Mar 24, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Updated (UTC)
open-tacos ⬜️ Ignored (Inspect) Visit Preview Apr 27, 2024 6:52am

@clintonlunn
Copy link
Collaborator Author

clintonlunn commented Mar 24, 2024

@vnugent it looks like there is an issue with shallow routing in Next v13. I think it has been fixed in canary release, or maybe even a next v14 release. I used a more manual approach of using the window.history.pushState() method. which mostly works in updating the page url when the user pans around, but if you press your mouse back and forward buttons, the entire page reloads (but it reloads at the correct position and zoom).

It seems like we need to use useRouter from next/navigation, instead of the previous next/router. And I don't think the router in this version of next has any shallow routing.

vercel/next.js#58335

Not very familiar with the routing in nextjs, so hopefully I am missing something!

@vnugent
Copy link
Contributor

vnugent commented Mar 26, 2024

It seems like we need to use useRouter from next/navigation, instead of the previous next/router. And I don't think the router in this version of next has any shallow routing.

Correct. I haven't investigated whether there are breaking changes in Next 14.

I did some research and came across this long discussion

Can you see if next-usequerystate library helps? (link in the comment above)

@clintonlunn
Copy link
Collaborator Author

@vnugent I pulled that library in and tested it a bit, but the behavior is the same. I am not sure if we need it. I checked some other websites and it seems our behavior matches. OSM has the same behavior where a back button click will reload the entire page at the new (previous) location. In addition, if we make this behavior work, it would prevent us from navigating away from the page with our back button. We'd have to click a the Home button or something

In addion, I have a different url format than OSM:

https://www.openstreetmap.org/#map=5/38.007/-95.844
versus
https://openbeta.io/maps?zoom=5&center=38.007%2C-95.844

Do you think it would be better for me to switch to how OSM does it or keep as is? As you can see I am using URLSearchParams() to generate the url which I figure might be sufficient.

@vnugent
Copy link
Contributor

vnugent commented Apr 1, 2024

I think it's perfectly fine using query string instead of the the # sign.

Browsing history. Google and Bing maps have this behavior:

  1. Clicking on a point of interest (PoI): the map keeps a history which allows you to go back with the Back button (also the browser doesn't do a full refresh).
  2. Panning, zooming, clicking on random locations (non PoI's): the map doesn't keep a history

we're not supporting ?placeId=<some_id> in the url at the moment so I think option 2 is fine. What do you think?

@clintonlunn
Copy link
Collaborator Author

@vnugent sorry for leaving this for a while. i am not sure how google and bing is able to intercept the back button in order to prevent the refresh. we are able to detect the back button by listening to the 'popstate' event, but I cannot seem to prevent the page from refreshing.

Regarding what you said about option 2. what do you mean? Do you want to not track the lat lng values unless the user has clicked on a poi?

Also looks like my slowness resulted in another pr getting opened. if that solution is preferable this is fine to be closed.

@vnugent
Copy link
Contributor

vnugent commented Apr 29, 2024

@clintonlunn I went ahead and merged (#1125). The back button is still a minor issue but we can live with it for now. Thanks for your work on this PR.

@vnugent vnugent closed this Apr 29, 2024
@clintonlunn
Copy link
Collaborator Author

@vnugent Glad it got sorted. I'll pick another issue and make sure to follow through with it.

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.

"Remember" map center and zoom level
2 participants