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

enable opening a single shared file #1242

Merged
merged 6 commits into from
Aug 26, 2022
Merged

Conversation

vanpertsch
Copy link
Contributor

@vanpertsch vanpertsch commented May 11, 2022

Signed-off-by: Vanessa Pertsch vanessa.pertsch@posteo.de

First steps towards #644 together with nextcloud/server#33347

@skjnldsv skjnldsv added enhancement New feature or request 2. developing Work in progress labels May 11, 2022
@skjnldsv skjnldsv added this to the Nextcloud 25 milestone May 11, 2022
@max-nextcloud
Copy link
Contributor

@vanpertsch there's some lint issues still in my commit. Sorry for that - did not clean it up. Could you fix them?

@vanpertsch vanpertsch force-pushed the enable-share-single-file branch 2 times, most recently from dc8006f to 035136d Compare May 18, 2022 14:18
src/views/Viewer.vue Outdated Show resolved Hide resolved
src/services/Viewer.js Outdated Show resolved Hide resolved
src/services/Viewer.js Outdated Show resolved Hide resolved
src/utils/fileUtils.js Outdated Show resolved Hide resolved
@vanpertsch vanpertsch force-pushed the enable-share-single-file branch 2 times, most recently from 7dbc84c to 6043727 Compare May 23, 2022 14:26
@PVince81
Copy link
Member

PVince81 commented Jun 8, 2022

@max-nextcloud would you be able to take over and finish this PR ?

@max-nextcloud
Copy link
Contributor

@PVince81 I won't have time for it this week - but i can get to it next week.

@max-nextcloud
Copy link
Contributor

I rebased it for now. I'll look into adding a test tomorrow. Other than that i think it should be done.

Copy link
Contributor

@artonge artonge 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, but not sure why we need to introduce the possibility to render the viewer in another element?

@skjnldsv
Copy link
Member

skjnldsv commented Jun 21, 2022

not sure why we need to introduce the possibility to render the viewer in another element?

e.g. using viewer for single-files public shares.

@PVince81
Copy link
Member

@max-nextcloud if you're done adding the test please set the label to "3 - To review" and ask for another review

@PVince81
Copy link
Member

/compile amend /

@PVince81
Copy link
Member

@max-nextcloud is this ready for re-review ?

@max-nextcloud
Copy link
Contributor

max-nextcloud commented Aug 11, 2022

@max-nextcloud is this ready for re-review ?

Yes.

As i said in #1242 (comment) - i'm not sure how best to resolve the dependency between this change and the one in files_server. Other than that i think this is good to go.

@skjnldsv
Copy link
Member

cypress test requires nextcloud/server#33347 to pass - which in turn depends on this to work. Not sure how best to resolve this.

I'd appreciate some reviews on the general approach as a start. Can probably clean up a bunch more stuff that is not used anymore now. Tried to get it working as a start.

Let's merge this one then server

@szaimen
Copy link
Contributor

szaimen commented Aug 17, 2022

@skjnldsv is testing needed? I could do with

docker run -it \
-p 127.0.0.1:8443:443 \
-e VIEWER_BRANCH=enable-share-single-file \
-e SERVER_BRANCH=feature/use-viewer-for-singe-file-shares \
--name nextcloud-easy-test \
ghcr.io/szaimen/nextcloud-easy-test:latest

@PVince81
Copy link
Member

@max-nextcloud server PR nextcloud/server#33347 got merged

Signed-off-by: Vanessa Pertsch <vanessa.pertsch@posteo.de>
@max-nextcloud
Copy link
Contributor

@max-nextcloud server PR nextcloud/server#33347 got merged

oh my... should have made it more clear it depends on this...

Rebasing this now so we can get it in fast.

max-nextcloud and others added 4 commits August 26, 2022 10:05
Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: Vanessa Pertsch <vanessa.pertsch@nextcloud.com>
Signed-off-by: Max <max@nextcloud.com>
This enables scrolling single file share views
so the download button below an image becomes visible.

Signed-off-by: Max <max@nextcloud.com>
@max-nextcloud
Copy link
Contributor

/compile amend .

@max-nextcloud
Copy link
Contributor

/compile amend /

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
@max-nextcloud max-nextcloud merged commit c51a5bf into master Aug 26, 2022
@max-nextcloud max-nextcloud deleted the enable-share-single-file branch August 26, 2022 11:08
Comment on lines +176 to +179
// only change zoomRatio when multiple files are in the fileList. Disable for single shared files
if (this.fileList.length > 1) {
this.zoomRatio = newZoomRatio
}
Copy link
Member

Choose a reason for hiding this comment

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

@max-nextcloud We can load a single file without being inline.
This is a regression

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. I'll fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

So i guess a good criterion will be if el is set.

max-nextcloud added a commit that referenced this pull request Sep 6, 2022
Zoom should only be disabled on single shared files.
See #1242 (review)

Signed-off-by: Max <max@nextcloud.com>
nextcloud-command pushed a commit that referenced this pull request Sep 6, 2022
Zoom should only be disabled on single shared files.
See #1242 (review)

Signed-off-by: Max <max@nextcloud.com>
Signed-off-by: nextcloud-command <nextcloud-command@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants