Skip to content

feat(web): Map in albums & shared albums #17906

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

Merged
merged 14 commits into from
May 5, 2025

Conversation

davidpcrd
Copy link
Contributor

Description

The goal of this PR is to add a map in albums and shared albums so that a user receiving a link can quickly locate the photos.
This is ideal for sharing a trip without needing to check the location of each photo individually.

To do this, I added a map button in /albums and /share.
When clicked, a modal window opens with a map showing markers for photos/videos.
Clicking on a marker allows viewing them just like in the /map page.

To implement this change, a new component album-map.svelte was created, taking album and isInMapView as arguments.
The second property was useful because there are two AssetViewer instances, and they were interfering with each other, causing unintended behaviors.
Following this idea, I also added the same variable to asset-grid.svelte.

How Has This Been Tested?

I tested that viewing photos in the album still works, as well as viewing them on the map.
I also verified that a shared album with hidden metadata does not display any location information.
Additionally, no extra photos are shown: only the photos associated with a marker are displayed.

  • npm run lint
  • npm run format
  • npm run check:svelte
  • npm run check:typescript
  • npm run test

Screenshots

album-main
album-map
share-main

Checklist:

  • I have performed a self-review of my own code
  • I have no unrelated changes in the PR.
  • I have confirmed that any new dependencies are strictly necessary.
  • I have written tests for new code (if applicable)
  • I have followed naming conventions/patterns in the surrounding code
  • All code in src/services/ uses repositories implementations for database calls, filesystem operations, etc.
  • All code in src/repositories/ is pretty basic/simple and does not have any immich specific logic (that belongs in src/services/)

@davidpcrd davidpcrd changed the title [Feature] Map in albums & shared albums (feat)web Map in albums & shared albums Apr 27, 2025
@davidpcrd davidpcrd changed the title (feat)web Map in albums & shared albums feat(web) Map in albums & shared albums Apr 27, 2025
@davidpcrd davidpcrd changed the title feat(web) Map in albums & shared albums feat(web): Map in albums & shared albums Apr 27, 2025
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason you cannot reuse the existing map?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean ? I use the <Map ... /> object on line 127 in album-map.svelte,

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry I assumed we had more logic from the map +page.svelte in the map component, but apparently we don't. That's kinda ugly, but not your problem then 😅

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see... I've taken a lot from /map logic. Sorry for this ugly code 🤦‍♂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was saying that how the map page currently is built is ugly lol.

@@ -32,6 +33,8 @@

let { isViewing: showAssetViewer } = assetViewingStore;

let isInMapView = $state(false);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help refactoring this boolean state to a AlbumMapViewManager global state class? It will look nicer because we will not use props drilling and binding.

You can see an example here

https://github.com/immich-app/immich/pull/17926/files#diff-dd054820b3629ac6faea866062e6a68a791348a504eb61eb3e51c37e374dc768

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

working on it. I've tried something, but not tested yet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be good 👌


onDestroy(() => {
abortController?.abort();
abortController?.abort();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate code

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done !

@alextran1502 alextran1502 enabled auto-merge (squash) May 5, 2025 02:51
@alextran1502 alextran1502 merged commit 7d61ed7 into immich-app:main May 5, 2025
45 checks passed
savely-krasovsky pushed a commit to savely-krasovsky/immich that referenced this pull request Jun 8, 2025
* add btn, map and marker

* Fix bug in navigation assetviewer

* Correct bug on main Viewer

* Add to user album the map of his pictures

* change icon to outline

* lint & format

* with manager instead of variable

* remove duplicate

* chore: minor styling change

* formatting

---------

Co-authored-by: Alex Tran <alex.tran1502@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants