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
Fix public link preview #9009
Fix public link preview #9009
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
42620dc
to
2ec7969
Compare
2ec7969
to
b85712c
Compare
@@ -23,7 +23,7 @@ export default defineComponent({ | |||
}, | |||
setup() { | |||
const resource = inject<Resource>('resource') | |||
const space = inject<SpaceResource>('space') | |||
const space = inject<Ref<SpaceResource>>('space') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here the injected space was correctly used (with unref
) but the type was wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you fix the type for resource
one line above as well while you're at it (same in FileDetails.vue
)? 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Funny enough, the resource
is provided as Resource
, not as Ref
. I don't know why we do that... but at least the type here is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh boy, you're right, see https://github.com/owncloud/web/blob/admin-settings-pagination/packages/web-app-files/src/components/SideBar/SideBar.vue#L296
We should align that some time in the future...
preview.value = yield previewService.loadPreview({ | ||
space, | ||
space: unref(space), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is the actual fix in this PR. The space
is provided by the sidebar as computed, but we were not accessing the value but the ref itself.
@@ -23,7 +23,7 @@ export default defineComponent({ | |||
}, | |||
setup() { | |||
const resource = inject<Resource>('resource') | |||
const space = inject<SpaceResource>('space') | |||
const space = inject<Ref<SpaceResource>>('space') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you fix the type for resource
one line above as well while you're at it (same in FileDetails.vue
)? 🙂
SonarCloud Quality Gate failed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One small improvement idea, otherwise LGTM 👍🏼
resource, | ||
dimensions: ImageDimension.Preview | ||
}) | ||
}).restartable() | ||
const isPreviewLoading = computed(() => { | ||
if (unref(isFolder)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can be reduced into one return statement, but im fine with this too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noo I like the easier readability this way
Description
Right sidebar / single file resource details file preview was not working at all in public links.
Types of changes
Checklist: