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

Automatically jumps to the next photo when deleting while previewing. #1143

Merged
merged 4 commits into from
Nov 23, 2020
Merged

Automatically jumps to the next photo when deleting while previewing. #1143

merged 4 commits into from
Nov 23, 2020

Conversation

chief8192
Copy link
Contributor

Description
Deletions made while previewing now immediately load the preview of the next file in the list. When the last file is deleted, previewing ends and returns to the file list, as before.

Closes #1091

@o1egl
Copy link
Member

o1egl commented Nov 11, 2020

@ramiresviana could you please review it?

Copy link
Contributor

@ramiresviana ramiresviana left a comment

Choose a reason for hiding this comment

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

There is an issue when pressing enter on delete dialog, the next() is being triggered. This should be solved by:

frontend/src/components/prompts/Delete.vue Outdated Show resolved Hide resolved
frontend/src/components/files/Preview.vue Outdated Show resolved Hide resolved
frontend/src/components/files/Preview.vue Show resolved Hide resolved
@chief8192
Copy link
Contributor Author

The problem I'm running into is that after omitting $root from this.$root.$emit(...) and this.$root.$on(...), event transmission stops working. Seems like it could be because Delete and Preview are sibling components?

@ramiresviana
Copy link
Contributor

Just omitting $root from this.$root.$on(...) is not going to work, you need to use v-on directive on the components you want to listen: https://vuejs.org/v2/guide/events.html

@chief8192
Copy link
Contributor Author

Yeah, so I've tried using v-on, in a number of different places. I can verify that the this.$emit('preview-deleted') call is working, because the event shows up in the devtools console when I click "DELETE".

The problem seems to be that prompts/Delete.vue is not a child component of files/Preview.vue. If I'm understanding this article correctly, the v-on would have to be added to the <delete> tag that gets generated in prompts/Prompts.vue. It doesn't look like that could be done without a decent amount of refactoring, and even that wouldn't gain us the ability to interact with Preview.vue.

@ramiresviana
Copy link
Contributor

Every element that is on template body of the component is considered a child, so <delete-button> is child of the preview. A working example is the <preview-size-button>, it has @change-size event and is being used on the preview.

@chief8192
Copy link
Contributor Author

I agree, what you're saying is true. The difference here is that while the Delete button is a child of the Preview component, the Delete prompt is not. And it's the Delete prompt which is ultimately responsible for making the API call to delete the file, and then calling $emit().

So given the limitation here that an event emitted by a child component has to be handled by the parent component (either via v-on or @event-name), I'm not clear how this could be easily made to work, given that the parent component of the Delete prompt is Prompts.vue, not Preview.vue:

filebrowser_components

@ramiresviana
Copy link
Contributor

I see now, since VueJS does not provide an proper solution for that case, i think we can use the $root instance.

@chief8192
Copy link
Contributor Author

Ok, committed a new round of changes

Copy link
Contributor

@ramiresviana ramiresviana left a comment

Choose a reason for hiding this comment

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

Navigation stops working when renaming file on preview, can you take a look on that? Similarly to the prompts/Delete you will need to create an event for prompts/Rename.

frontend/src/components/files/Preview.vue Show resolved Hide resolved
frontend/src/components/files/Preview.vue Outdated Show resolved Hide resolved
frontend/src/components/files/Preview.vue Outdated Show resolved Hide resolved
@chief8192
Copy link
Contributor Author

I did a clean build from master, and the rename issue is present there as well, so it looks to be an unrelated issue out of scope for this PR.

Copy link
Contributor

@ramiresviana ramiresviana left a comment

Choose a reason for hiding this comment

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

Looking good, thank you for contributing to this project.

@o1egl o1egl merged commit 9515cee into filebrowser:master Nov 23, 2020
@chief8192
Copy link
Contributor Author

Cheers, happy to help!

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.

Go to next Image preview when deleting an Image
3 participants