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(server,web): hide faces #3262

Merged
merged 31 commits into from
Jul 18, 2023
Merged

Conversation

martabal
Copy link
Member

@martabal martabal commented Jul 14, 2023

This PR adds the ability to hide people from asset viewer & explore page.

To do:

  • move the logic from the client to server
  • fix bugs
  • simplify the user-page-layout
  • add server tests
2023-07-16.22-57-37.mp4

@vercel
Copy link

vercel bot commented Jul 14, 2023

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 Jul 18, 2023 6:05pm

Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Looks like a good start. A few notes:

  • We can definitely filter out people when we get asset details. That could even be in the response dto mapper for now.
  • I think the people endpoint needs to be able to return hidden people, so that we can use it to show a list that the user can toggle, although we do want to hide them on the explore page. We can keep filtering that client-side or have a query param to show hidden, and exclude them by default.
  • In general we have isAdmin, isArchived, isFavorite, isVisible, etc. Maybe it would be more consistent to have this be called isHidden.

server/test/fixtures.ts Outdated Show resolved Hide resolved
web/src/lib/components/asset-viewer/detail-panel.svelte Outdated Show resolved Hide resolved
@martabal martabal force-pushed the feat/hide-faces branch 3 times, most recently from 636e0e5 to f05ae0b Compare July 14, 2023 17:14
@martabal
Copy link
Member Author

Looks like a good start. A few notes:

* We can definitely filter out people when we get asset details. That could even be in the response dto mapper for now.

* I think the people endpoint needs to be able to return hidden people, so that we can use it to show a list that the user can toggle, although we do want to hide them on the explore page. We can keep filtering that client-side or have a query param to show hidden, and exclude them by default.

* In general we have `isAdmin`, `isArchived`, `isFavorite`, `isVisible`, etc. Maybe it would be more consistent to have this be called `isHidden`.

Thank you for your feedback !

@martabal martabal force-pushed the feat/hide-faces branch 2 times, most recently from 374fc40 to 56853f7 Compare July 15, 2023 14:29
@martabal martabal changed the title feat: hide faces feat(server,web): hide faces Jul 15, 2023
@martabal
Copy link
Member Author

martabal commented Jul 15, 2023

I added more use cases with view all and show & hide faces buttons

2023-07-16.12-03-22.mp4

@alextran1502
Copy link
Contributor

The latest video doesn't indicate that if you hide faces, how can you unhide them?

@martabal
Copy link
Member Author

martabal commented Jul 16, 2023

The latest video doesn't indicate that if you hide faces, how can you unhide them?

The notification is hiding the Show & hide faces button which allow to hide and unhide faces, I have updated the video

server/src/domain/person/person.service.ts Outdated Show resolved Hide resolved
server/src/immich/api-v1/asset/asset.service.ts Outdated Show resolved Hide resolved
server/src/immich/controllers/person.controller.ts Outdated Show resolved Hide resolved
server/src/immich/controllers/person.controller.ts Outdated Show resolved Hide resolved
server/src/immich/controllers/person.controller.ts Outdated Show resolved Hide resolved
web/src/routes/(user)/explore/+page.server.ts Outdated Show resolved Hide resolved
web/src/routes/(user)/people/+page.svelte Outdated Show resolved Hide resolved
@jrasm91
Copy link
Contributor

jrasm91 commented Jul 16, 2023

I think that is ok. It is unlikely to be a problem unless they're doing 100 or more. Even then, they're probably only going to do it once. So I think it's ok to not optimize by adding a new endpoint.

@martabal martabal marked this pull request as ready for review July 17, 2023 20:37
server/src/domain/person/person.dto.ts Outdated Show resolved Hide resolved
server/src/domain/person/person.dto.ts Outdated Show resolved Hide resolved
server/src/domain/person/person.service.spec.ts Outdated Show resolved Hide resolved
server/src/domain/person/person.service.ts Outdated Show resolved Hide resolved
server/src/domain/person/person.service.ts Outdated Show resolved Hide resolved
server/src/immich/controllers/person.controller.ts Outdated Show resolved Hide resolved
server/src/immich/controllers/person.controller.ts Outdated Show resolved Hide resolved
web/src/lib/components/layouts/user-page-layout.svelte Outdated Show resolved Hide resolved
web/src/lib/components/layouts/user-page-layout.svelte Outdated Show resolved Hide resolved
web/src/lib/components/layouts/user-page-layout.svelte Outdated Show resolved Hide resolved
@martabal martabal force-pushed the feat/hide-faces branch 2 times, most recently from 329dac9 to d9d7433 Compare July 18, 2023 09:52
Copy link
Contributor

@jrasm91 jrasm91 left a comment

Choose a reason for hiding this comment

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

Code looks good to me.

@alextran1502 alextran1502 merged commit f28fc8f into immich-app:main Jul 18, 2023
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