Skip to content

feat(web): add link to external map in leaflet popup - #3847

Merged
jrasm91 merged 2 commits into
immich-app:mainfrom
lefuturiste:main
Aug 25, 2023
Merged

jrasm91 merged 2 commits into
immich-app:mainfrom
lefuturiste:main

Conversation

@lefuturiste

@lefuturiste lefuturiste commented Aug 23, 2023 •

Copy link
Copy Markdown
Contributor

A little life improvement.

Sometimes it's useful to open a geo location to an external map application to not have to copy the coordinates manually. Here I put a link to OpenStreetMap because it's what I personally use. But I known some people would want to use something different. We could instead link to geohacks (eg. https://geohack.toolforge.org/geohack.php?params=048.861085_N_0002.313158_E_globe:Earth) or make it a configurable param.

I'm eager to hear your takes on this.

image

Sometimes it's useful to open a geo location to an external map
application to not have to copy the coordinates manually.
Here I put a link to OpenStreetMap because it's what I personally use.
But I known some people would want to use something different. We could
instead link to geohacks (eg. https://geohack.toolforge.org/geohack.php?params=048.861085_N_0002.313158_E_globe:Earth)
or make it a configurable param.
@vercel

vercel Bot commented Aug 23, 2023 •

Copy link
Copy Markdown

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
immich ⬜️ Ignored (Inspect) Visit Preview Aug 25, 2023 1:16pm

Comment thread web/src/lib/components/shared-components/leaflet/marker.svelte
@lefuturiste
lefuturiste marked this pull request as ready for review August 23, 2023 11:02
@jrasm91

jrasm91 commented Aug 23, 2023

Copy link
Copy Markdown
Member

Can you add some screenshots or video to the PR description?

@lefuturiste

Copy link
Copy Markdown
Contributor Author

Can you add some screenshots or video to the PR description?

done

@jrasm91

jrasm91 commented Aug 23, 2023

Copy link
Copy Markdown
Member

I'm not a huge fan of the marker with text personally. We could instead display the lat/long in the info panel along with the address and/or wrap the whole leaflet map with a link so clicking on it links to a map.

@lefuturiste

Copy link
Copy Markdown
Contributor Author

I'm not a huge fan of the marker with text personally. We could instead display the lat/long in the info panel along with the address and/or wrap the whole leaflet map with a link so clicking on it links to a map.

I'm not sure about the link on the whole map, I think that the map should be at minimum interactive as it's the case right now. Being able to wander a little bit around, zoom out or zoom in to locate the place is important. I think the best is to have an explicit link with an explicit label, so the user known whats the purpose.

For putting the coordinates in the infos panel, why not, but where to put it? I don't think we have the room to put it on the same field with the locality (reversed geocoding result) .

@alextran1502

Copy link
Copy Markdown
Member

I believe right now you can click on the icon to show the lat/lon right? Why don't you add a link to the pop up instead of always show it? It would make the UI cleaner and also serve your needs

@lefuturiste

Copy link
Copy Markdown
Contributor Author

Yes, I didn't change the behavior, you still need to click on the marker for the popup to show up (perharps I should have recorded a video)

Comment thread web/src/lib/components/shared-components/leaflet/marker.svelte
@jrasm91
jrasm91 enabled auto-merge (squash) August 25, 2023 13:17
@jrasm91
jrasm91 merged commit 20e0c03 into immich-app:main Aug 25, 2023
martabal pushed a commit that referenced this pull request Sep 11, 2023
* feat(web): add link to external map in leaflet popup

Sometimes it's useful to open a geo location to an external map
application to not have to copy the coordinates manually.
Here I put a link to OpenStreetMap because it's what I personally use.
But I known some people would want to use something different. We could
instead link to geohacks (eg. https://geohack.toolforge.org/geohack.php?params=048.861085_N_0002.313158_E_globe:Earth)
or make it a configurable param.

* chore: cleanup

---------

Co-authored-by: Jason Rasmussen <jrasm91@gmail.com>
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.

3 participants