fix: prevent trashing of trashed assets #10028
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roschaefer
commented
Jun 7, 2024
| ]; | ||
|
|
||
| if (!asset.isTrashed) { | ||
| shortcutOptions.push({ shortcut: { key: 'Delete' }, onShortcut: () => trashOrDelete(false) }); |
There was a problem hiding this comment.
This one is not tested. I could - but I would refactor the callbacks to live outside the component, for easier testing. Shall I?
roschaefer
force-pushed
the
fix-prevent-obsolete-trash-action
branch
2 times, most recently
from
dbb9694
to
0d8daa4
Compare
|
So how can I add PR labels as a contributor? |
web/src/lib/components/asset-viewer/asset-viewer-nav-bar.spec.ts
Outdated
Show resolved
Hide resolved
web/src/lib/components/asset-viewer/asset-viewer-nav-bar.svelte
Outdated
Show resolved
Hide resolved
Motivation ---------- This will improve user experience by hiding a pointless action. You can not trash a trashed asset again. It won't get any trashier than it already is. How to test ----------- 1. Visit route `/trash` 2. Click on an asset 3. Press "Delete" on your keyboard 4. Nothing happens 5. Try to find the trash button in the top right 6. You can't find it
|
@michelheusschen thanks for the review. I hope I could address all the requested changes? |
roschaefer
force-pushed
the
fix-prevent-obsolete-trash-action
branch
from
0d8daa4
to
c9654cd
Compare
roschaefer
added a commit
to roschaefer/immich
that referenced
this pull request
Jun 8, 2024
Motivation ---------- While working on immich-app#10028 I would see the following: ``` $ git status On branch gitignore-docker-postgres-volume Untracked files: (use "git add <file>..." to include in what will be committed) docker/postgres/ nothing added to commit but untracked files present (use "git add" to track) ``` Why was the postgres bound volume not git-ignored? Another question: Should we use one `.gitignore` in the root directory or many `.gitignore` files in each project subdirectory? I'm asking because I can see a `.gitignore` in `docker/` which ignores `.env`. I would prefer dedicated `.gitignore` files per project directory. How to test ----------- 1. `cd docker/` 2. `docker compose -f docker-compose.dev.yml up` 3. Wait.. 4. `git status` 5. nothing to commit, working tree clean
michelheusschen
approved these changes
Jun 8, 2024
roschaefer
added a commit
to roschaefer/immich
that referenced
this pull request
Jun 8, 2024
Motivation ---------- It's a follow up to immich-app#10028. I think it would be better user experience if one can tell by the icon what the delete button is about to do. I hope I caught all the occurences where one can permanently delete assets. How to test ----------- 1. Visit e.g. `/trash` 2. If you select some assets, the delete button in the top right corner looks different.
alextran1502
approved these changes
Jun 8, 2024
roschaefer
added a commit
to roschaefer/immich
that referenced
this pull request
Jun 9, 2024
Motivation ---------- It's a follow up to immich-app#10028. I think it would be better user experience if one can tell by the icon what the delete button is about to do. I hope I caught all the occurences where one can permanently delete assets. How to test ----------- 1. Visit e.g. `/trash` 2. If you select some assets, the delete button in the top right corner looks different.
alextran1502
pushed a commit
that referenced
this pull request
Jun 9, 2024
Motivation ---------- It's a follow up to #10028. I think it would be better user experience if one can tell by the icon what the delete button is about to do. I hope I caught all the occurences where one can permanently delete assets. How to test ----------- 1. Visit e.g. `/trash` 2. If you select some assets, the delete button in the top right corner looks different.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This will improve user experience
by hiding a pointless actionby changing "move to trash" into "delete permanently" if the asset is already in the trash.You can not trash a trashed asset again. It won't get any trashier than it already is.How to test
/trash