search

OpenBeta / open-tacos

Rock climbing route catalog (openbeta.io)
https://openbeta.io
GNU Affero General Public License v3.0
132 stars 116 forks source link

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

Closed clintonlunn closed 5 months ago

clintonlunn commented 6 months ago

fixes #1113

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

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

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

vercel[bot] commented 6 months ago

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

1 Ignored Deployment | Name | Status | Preview | Updated (UTC) | | :--- | :----- | :------ | :------ | | **open-tacos** | ⬜️ Ignored ([Inspect](https://vercel.com/openbeta-dev/open-tacos/EtGSR8iWkdum5JJHsyLk2pVejbEM)) | [Visit Preview](https://open-tacos-git-map-url-openbeta-dev.vercel.app) | Apr 27, 2024 6:52am |
clintonlunn commented 6 months ago

@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.

https://github.com/vercel/next.js/pull/58335

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

vnugent commented 6 months ago

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 commented 6 months ago

@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 commented 6 months ago

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 commented 5 months ago

@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 commented 5 months ago

@clintonlunn I went ahead and merged (https://github.com/OpenBeta/open-tacos/pull/1125). The back button is still a minor issue but we can live with it for now. Thanks for your work on this PR.

clintonlunn commented 4 months ago

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