vikunja
/
vikunja
6
43
Fork 72
Code Issues 151 Pull Requests 19 Packages Releases 32 Activity

Frontend: Proof of concept for image preview #2266

Open
Elscrux wants to merge 9 commits from Elscrux/vikunja:feature/image-preview into main
pull from: Elscrux/vikunja:feature/image-preview
merge into: vikunja:main
Conversation 4 Commits 9 Files Changed 3 +134 -73
3 changed files with 134 additions and 73 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
frontend/cypress/e2e/task/task.spec.ts
View File

@ -42,7 +42,7 @@ function uploadAttachmentAndVerify(taskId: number) {
.selectFile('cypress/fixtures/image.jpg', {force: true}) // The input is not visible, but on purpose
cy.wait('@uploadAttachment')
cy.get('.attachments .attachments .files a.attachment')
cy.get('.attachments .attachments .files div.attachment')
.should('exist')
}

170
frontend/src/components/tasks/partials/attachments.vue
View File

@ -27,83 +27,89 @@
v-if="attachments.length > 0"
class="files"
>
<!-- FIXME: don't use a for element that wraps other links / buttons
Instead: overlay element with button that is inside.
-->
<a
<button
konrad commented 2024-05-05 14:01:52 +00:00
Outdated
Review
Copy Link

Please do not use a table for multi-column layout.

Please do not use a table for multi-column layout.
Elscrux commented 2024-05-09 10:30:11 +00:00
Outdated
Review
Copy Link

I used a grid instead now

I used a grid instead now
v-for="a in attachments"
:key="a.id"
class="attachment"
konrad commented 2024-05-16 07:10:19 +00:00
Outdated
Review
Copy Link

grid-item sounds like a child of a grid. Why not call it attachment as it was before?

`grid-item` sounds like a child of a grid. Why not call it `attachment` as it was before?
@click="viewOrDownload(a)"
>
<div class="filename">
{{ a.file.name }}
<span
v-if="task.coverImageAttachmentId === a.id"
class="is-task-cover"
>
{{ $t('task.attachment.usedAsCover') }}
</span>
<div class="preview-column">
<FilePreview
v-if="canPreview(a)"
class="attachment-preview"
:model-value="a"
/>
</div>
<div class="info">
<p class="attachment-info-meta">
<i18n-t
keypath="task.attachment.createdBy"
scope="global"
<div>
<div class="filename">
{{ a.file.name }}
<span
v-if="task.coverImageAttachmentId === a.id"
class="is-task-cover"
>
<span v-tooltip="formatDateLong(a.created)">
{{ formatDateSince(a.created) }}
{{ $t('task.attachment.usedAsCover') }}
</span>
</div>
<div class="info">
<p class="attachment-info-meta">
<i18n-t
keypath="task.attachment.createdBy"
scope="global"
>
<span v-tooltip="formatDateLong(a.created)">
{{ formatDateSince(a.created) }}
</span>
<User
:avatar-size="24"
:user="a.createdBy"
:is-inline="true"
/>
</i18n-t>
<span>
{{ getHumanSize(a.file.size) }}
</span>
konrad commented 2024-04-11 15:40:04 +00:00
Outdated
Review
Copy Link

I would extract this out into its own component, which takes the attachment, fetches the image and shows it next to the attachment. We could extend this further to show some kind of icon for pdfs or other file types later on.

I would extract this out into its own component, which takes the attachment, fetches the image and shows it next to the attachment. We could extend this further to show some kind of icon for pdfs or other file types later on.
Elscrux commented 2024-04-26 17:05:57 +00:00
Outdated
Review
Copy Link

Good point, I did that

Good point, I did that
<User
:avatar-size="24"
:user="a.createdBy"
:is-inline="true"
/>
</i18n-t>
<span>
{{ getHumanSize(a.file.size) }}
</span>
<span v-if="a.file.mime">
{{ a.file.mime }}
</span>
</p>
<p>
<BaseButton
v-tooltip="$t('task.attachment.downloadTooltip')"
class="attachment-info-meta-button"
@click.prevent.stop="downloadAttachment(a)"
>
{{ $t('misc.download') }}
</BaseButton>
<BaseButton
v-tooltip="$t('task.attachment.copyUrlTooltip')"
class="attachment-info-meta-button"
@click.stop="copyUrl(a)"
>
{{ $t('task.attachment.copyUrl') }}
</BaseButton>
<BaseButton
v-if="editEnabled"
v-tooltip="$t('task.attachment.deleteTooltip')"
class="attachment-info-meta-button"
@click.prevent.stop="setAttachmentToDelete(a)"
>
{{ $t('misc.delete') }}
</BaseButton>
<BaseButton
v-if="editEnabled"
class="attachment-info-meta-button"
@click.prevent.stop="setCoverImage(task.coverImageAttachmentId === a.id ? null : a)"
>
{{
task.coverImageAttachmentId === a.id
? $t('task.attachment.unsetAsCover')
: $t('task.attachment.setAsCover')
}}
</BaseButton>
</p>
<span v-if="a.file.mime">
{{ a.file.mime }}
</span>
</p>
<p>
<BaseButton
v-tooltip="$t('task.attachment.downloadTooltip')"
class="attachment-info-meta-button"
@click.prevent.stop="downloadAttachment(a)"
>
{{ $t('misc.download') }}
</BaseButton>
<BaseButton
v-tooltip="$t('task.attachment.copyUrlTooltip')"
class="attachment-info-meta-button"
@click.stop="copyUrl(a)"
>
{{ $t('task.attachment.copyUrl') }}
</BaseButton>
<BaseButton
v-if="editEnabled"
v-tooltip="$t('task.attachment.deleteTooltip')"
class="attachment-info-meta-button"
@click.prevent.stop="setAttachmentToDelete(a)"
>
{{ $t('misc.delete') }}
</BaseButton>
<BaseButton
v-if="editEnabled"
class="attachment-info-meta-button"
@click.prevent.stop="setCoverImage(task.coverImageAttachmentId === a.id ? null : a)"
>
{{
task.coverImageAttachmentId === a.id
? $t('task.attachment.unsetAsCover')
: $t('task.attachment.setAsCover')
}}
</BaseButton>
</p>
</div>
</div>
</a>
</button>
</div>
<x-button
@ -188,6 +194,7 @@ import {useCopyToClipboard} from '@/composables/useCopyToClipboard'
import {error, success} from '@/message'
import {useTaskStore} from '@/stores/tasks'
import {useI18n} from 'vue-i18n'
import FilePreview from '@/components/tasks/partials/file-preview.vue'
const {
task,
@ -260,13 +267,17 @@ async function deleteAttachment() {
const attachmentImageBlobUrl = ref<string | null>(null)
async function viewOrDownload(attachment: IAttachment) {
if (SUPPORTED_IMAGE_SUFFIX.some((suffix) => attachment.file.name.endsWith(suffix))) {
if (canPreview(attachment)) {
attachmentImageBlobUrl.value = await attachmentService.getBlobUrl(attachment)
} else {
downloadAttachment(attachment)
}
}
function canPreview(attachment: IAttachment): boolean {
return SUPPORTED_IMAGE_SUFFIX.some((suffix) => attachment.file.name.endsWith(suffix))
}
const copy = useCopyToClipboard()
function copyUrl(attachment: IAttachment) {
@ -298,11 +309,18 @@ async function setCoverImage(attachment: IAttachment | null) {
}
.attachment {
display: grid;
grid-template-columns: 9rem 1fr;
align-items: center;
margin-bottom: .5rem;
display: block;
transition: background-color $transition;
border-radius: $radius;
padding: .5rem;
transition: background-color $transition;
background-color: transparent;
border: transparent;
border-radius: $radius;
&:hover {
background-color: var(--grey-200);
@ -434,6 +452,14 @@ async function setCoverImage(attachment: IAttachment | null) {
}
}
.preview-column {
max-width: 8rem;
konrad commented 2024-05-16 07:11:45 +00:00
Outdated
Review
Copy Link

What happens when the item is clicked?

What happens when the item is clicked?
Elscrux commented 2024-05-16 12:30:58 +00:00
Outdated
Review
Copy Link

The image preview is shown, like previously. Nothing changed in that regard. Previously, an tag was used to have the cursor change to a clickable cursor. This comment was there and I fixed it by adding this clickable class.

FIXME: don't use a for element that wraps other links / buttons

The image preview is shown, like previously. Nothing changed in that regard. Previously, an <a> tag was used to have the cursor change to a clickable cursor. This comment was there and I fixed it by adding this clickable class. > FIXME: don't use a for element that wraps other links / buttons
konrad commented 2024-05-16 15:37:26 +00:00
Outdated
Review
Copy Link

In terms of a11y, if it's actually clickable, it should be a button or a. Divs are usually not clickable.

In terms of a11y, if it's actually clickable, it should be a button or a. Divs are usually not clickable.
Elscrux commented 2024-05-16 17:30:23 +00:00
Outdated
Review
Copy Link

Fair. I changed this and it has a little different feel to it now. Let me know what you think!

Fair. I changed this and it has a little different feel to it now. Let me know what you think!
konrad commented 2024-05-16 17:32:50 +00:00
Outdated
Review
Copy Link

Looks good! Will do a proper review when I'm back at my computer, early next week.

Looks good! Will do a proper review when I'm back at my computer, early next week.
❤️ 1
}
.attachment-preview {
max-height: 4rem;
}
.is-task-cover {
background: var(--primary);
color: var(--white);

35
frontend/src/components/tasks/partials/file-preview.vue Normal file
View File

@ -0,0 +1,35 @@
<template>
<img
konrad commented 2024-05-05 14:03:55 +00:00
Outdated
Review
Copy Link

Can you make this always the same size? Use object-fit: cover; to let the image fill the container nicely.

Can you make this always the same size? Use [`object-fit: cover;`](https://developer.mozilla.org/en-US/docs/Web/CSS/object-fit) to let the image fill the container nicely.
Elscrux commented 2024-05-09 10:30:04 +00:00
Outdated
Review
Copy Link

I included it, but it didn't seem like it made a difference

I included it, but it didn't seem like it made a difference
konrad commented 2024-05-15 21:37:01 +00:00
Outdated
Review
Copy Link

Did you set a fixed with and height for the image?

Did you set a fixed with and height for the image?
Elscrux commented 2024-05-15 21:56:04 +00:00
Outdated
Review
Copy Link

Yes, it's at 4rem now

Yes, it's at 4rem now
:src="blobUrl"
alt="Attachment preview"
>
konrad commented 2024-05-16 07:06:52 +00:00
Outdated
Review
Copy Link

Please do not use inline styles.

This image should have a rectangular fixed size, then the object-fit property will make sure it looks good.

Please do not use inline styles. This image should have a rectangular fixed size, then the `object-fit` property will make sure it looks good.
</template>
<script setup lang="ts">
import {type PropType, ref, shallowReactive, watchEffect} from 'vue'
import AttachmentService from '@/services/attachment'
import type { IAttachment } from '@/modelTypes/IAttachment'
const props = defineProps({
modelValue: {
type: Object as PropType<IAttachment>,
default: undefined,
},
})
const attachmentService = shallowReactive(new AttachmentService())
const blobUrl = ref<string | undefined>(undefined)
watchEffect(async () => {
if (props.modelValue) {
blobUrl.value = await attachmentService.getBlobUrl(props.modelValue)
}
})
</script>
<style scoped lang="scss">
img {
border-radius: $radius;
konrad marked this conversation as resolved Outdated
konrad commented 2024-05-05 14:02:42 +00:00
Outdated
Review
Copy Link

Please use $radius here so that the radius is consistent with other elements.

Please use `$radius` here so that the radius is consistent with other elements.
Elscrux commented 2024-05-09 10:29:53 +00:00
Outdated
Review
Copy Link

Done!

Done!
object-fit: cover;
}
</style>