feat(task): cover image for tasks #2460

Merged
konrad merged 10 commits from feature/task-cover into main 2022-10-05 13:28:13 +00:00
7 changed files with 169 additions and 81 deletions

View File

@ -9,7 +9,7 @@
<input
v-if="editEnabled"
:disabled="attachmentService.loading || undefined"
:disabled="loading || undefined"
@change="uploadNewAttachment()"
id="files"
multiple
@ -35,7 +35,15 @@
:key="a.id"
@click="viewOrDownload(a)"
>
<div class="filename">{{ a.file.name }}</div>
<div class="filename">
{{ a.file.name }}
<span
v-if="task.coverImageAttachmentId === a.id"
class="is-task-cover"
>
{{ $t('task.attachment.usedAsCover') }}
</span>
</div>
<div class="info">
<p class="attachment-info-meta">
<i18n-t keypath="task.attachment.createdBy" scope="global">
@ -78,6 +86,17 @@
>
konrad marked this conversation as resolved Outdated

Picky:
line wrap

Picky: line wrap

Done.

Done.
{{ $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>
</a>
@ -85,7 +104,7 @@
<x-button
v-if="editEnabled"
:disabled="attachmentService.loading"
:disabled="loading"
@click="filesRef?.click()"
class="mb-4"
icon="cloud-upload-alt"
@ -118,7 +137,7 @@
<template #header>
<span>{{ $t('task.attachment.delete') }}</span>
</template>
<template #text>
<p>
{{ $t('task.attachment.deleteText1', {filename: attachmentToDelete.file.name}) }}<br/>
@ -138,13 +157,14 @@
</template>
<script setup lang="ts">
import {ref, shallowReactive, computed, type PropType} from 'vue'
import {ref, shallowReactive, computed} from 'vue'
import {useDropZone} from '@vueuse/core'
import User from '@/components/misc/user.vue'
import BaseButton from '@/components/base/BaseButton.vue'
import AttachmentService from '@/services/attachment'
import {SUPPORTED_IMAGE_SUFFIX} from '@/models/attachment'
import type AttachmentModel from '@/models/attachment'
import type {IAttachment} from '@/modelTypes/IAttachment'

Because I never mentioned this I'm noting this down here.
I currently am following this order roughly when creating vue files:

  • imports grouped by
    • vue and vue ecosystem packages
    • other external and external styles of vendors
      - then loose groups of (depending on what makes sense)
      • components
      • helpers
      • stores
      • types
  • helper functions only used in this file
  • local constants
  • props then emits
  • inits of router, i18n, stores
  • init state
    • that is used at several locations in the component and
    • template refs
    • these can also be computeds
  • rest of file grouped by functionality. this can be functions composables computeds and refs or other stuff I forgot.

When I migrated to script setup I usually tried to at least follow this until init state.
I'm happy to use a different order or no order at all, just wanted to inform about my though process here :)


In case we want to follow this in the future: Should I mention this somewhere in the docs?

Because I never mentioned this I'm noting this down here. I currently am following this order roughly when creating vue files: - imports grouped by - vue and vue ecosystem packages - other external and external styles of vendors - then loose groups of (depending on what makes sense) - components - helpers - stores - types - helper functions only used in this file - local constants - props then emits - inits of router, i18n, stores - init state - that is used at several locations in the component and - template refs - these *can* also be computeds - rest of file grouped by functionality. this can be functions composables computeds and refs or other stuff I forgot. When I migrated to script setup I usually tried to at least follow this until init state. I'm happy to use a different order or no order at all, just wanted to inform about my though process here :) -------- In case we want to follow this in the future: Should I mention this somewhere in the docs?
import type {ITask} from '@/modelTypes/ITask'
konrad marked this conversation as resolved Outdated

add global scope

add global scope

Done.

Done.
@ -155,38 +175,44 @@ import {uploadFiles, generateAttachmentUrl} from '@/helpers/attachments'
import {getHumanSize} from '@/helpers/getHumanSize'
import {useCopyToClipboard} from '@/composables/useCopyToClipboard'
import {error, success} from '@/message'
import {useTaskStore} from '@/stores/tasks'
import {useI18n} from 'vue-i18n'

Maybe now that we have a two-way binding we should use v-model?

Maybe now that we have a two-way binding we should use `v-model`?

I didn't do that because I feel like a v-model for the attachment component would be the attachments itself. But we need to pass data to the parent task, not the attachments.

I'd like to refactor the task detail view so that the task in there is loaded in through the store and all updates happen through the store. That way we would not need a v-model here and solve things like this and this.

I didn't do that because I feel like a `v-model` for the attachment component would be the attachments itself. But we need to pass data to the parent task, not the attachments. I'd like to refactor the task detail view so that the task in there is loaded in through the store and all updates happen through the store. That way we would not need a `v-model` here and solve things like [this](https://my.vikunja.cloud/tasks/16615) and [this](https://my.vikunja.cloud/tasks/16619).

Let's plan a bit when we do this, but sounds good :)

Let's plan a bit when we do this, but sounds good :)

Just that I understand correctly:
So in this case you would want to pass in the taskId again in the future.
And to manage the attachments we would call store methods from inside the component?

Just that I understand correctly: So in this case you would want to pass in the taskId again in the future. And to manage the attachments we would call store methods from inside the component?

Or maybe pass the whole task object into the component, but don't bubble it back up but instead update it through the store.

Or maybe pass the whole task object into the component, but don't bubble it back up but instead update it through the store.
const props = defineProps({
taskId: {
type: Number as PropType<ITask['id']>,
required: true,
},
initialAttachments: {
type: Array,
},
editEnabled: {
default: true,
},
const taskStore = useTaskStore()
const {t} = useI18n({useScope: 'global'})
konrad marked this conversation as resolved Outdated

The reason I do use shallowReactive for services is: we can easily watch the loading state.
I'm not even sure if we have a deep nested state in any service so maybe reactive would actually be identical.
My thought was here only that in order to have loading reactive shallowReactive is enough.

The reason I do use `shallowReactive` for services is: we can easily watch the loading state. I'm not even sure if we have a deep nested state in any service so maybe reactive would actually be identical. My thought was here only that in order to have `loading` reactive `shallowReactive` is enough.
const props = withDefaults(defineProps<{
task: ITask,
initialAttachments?: IAttachment[],
editEnabled: boolean,
}>(), {
editEnabled: true,
})
// FIXME: this should go through the store
const emit = defineEmits(['task-changed'])
const attachmentService = shallowReactive(new AttachmentService())
const attachmentStore = useAttachmentStore()
const attachments = computed(() => attachmentStore.attachments)
const loading = computed(() => attachmentService.loading || taskStore.isLoading)
function onDrop(files: File[] | null) {
if (files && files.length !== 0) {
uploadFilesToTask(files)
}
}
const { isOverDropZone } = useDropZone(document, onDrop)
const {isOverDropZone} = useDropZone(document, onDrop)
function downloadAttachment(attachment: IAttachment) {
attachmentService.download(attachment)
}
const filesRef = ref<HTMLInputElement | null>(null)
function uploadNewAttachment() {
const files = filesRef.value?.files
@ -198,7 +224,7 @@ function uploadNewAttachment() {
}
function uploadFilesToTask(files: File[] | FileList) {
uploadFiles(attachmentService, props.taskId, files)
uploadFiles(attachmentService, props.task.id, files)
}
const attachmentToDelete = ref<AttachmentModel | null>(null)
@ -217,16 +243,15 @@ async function deleteAttachment() {
attachmentStore.removeById(attachmentToDelete.value.id)
success(r)
setAttachmentToDelete(null)
} catch(e) {
} catch (e) {
error(e)
}
}
const attachmentImageBlobUrl = ref<string | null>(null)
const SUPPORTED_SUFFIX = ['.jpg', '.png', '.bmp', '.gif']
async function viewOrDownload(attachment: AttachmentModel) {
if (SUPPORTED_SUFFIX.some((suffix) => attachment.file.name.endsWith(suffix)) ) {
if (SUPPORTED_IMAGE_SUFFIX.some((suffix) => attachment.file.name.endsWith(suffix))) {
attachmentImageBlobUrl.value = await attachmentService.getBlobUrl(attachment)
konrad marked this conversation as resolved Outdated

Put this function as named exported function in the task store.
This way we can in theory reuse this whereever we want.
Having all actions like these in the store might help us in the future to easier implement such functionality with quick actions.

Obviously the function in the store then needs to accept the task additionally to the attachment as paramenter.

Put this function as named exported function in the task store. This way we can in theory reuse this whereever we want. Having all actions like these in the store might help us in the future to easier implement such functionality with quick actions. Obviously the function in the store then needs to accept the task additionally to the attachment as paramenter.

Done.

Done.
} else {
downloadAttachment(attachment)
@ -234,8 +259,15 @@ async function viewOrDownload(attachment: AttachmentModel) {
}
const copy = useCopyToClipboard()
function copyUrl(attachment: IAttachment) {
copy(generateAttachmentUrl(props.taskId, attachment.id))
copy(generateAttachmentUrl(props.task.id, attachment.id))
}
async function setCoverImage(attachment: IAttachment | null) {
const task = await taskStore.setCoverImage(props.task, attachment)
emit('task-changed', task)
success({message: t('task.attachment.successfullyChangedCoverImage')})
}
</script>
@ -316,7 +348,7 @@ function copyUrl(attachment: IAttachment) {
height: auto;
text-shadow: var(--shadow-md);
animation: bounce 2s infinite;
@media (prefers-reduced-motion: reduce) {
animation: none;
}
@ -338,7 +370,7 @@ function copyUrl(attachment: IAttachment) {
.attachment-info-meta {
display: flex;
align-items: center;
:deep(.user) {
display: flex !important;
align-items: center;
@ -348,7 +380,7 @@ function copyUrl(attachment: IAttachment) {
@media screen and (max-width: $mobile) {
flex-direction: column;
align-items: flex-start;
:deep(.user) {
margin: .5rem 0;
}
@ -394,5 +426,13 @@ function copyUrl(attachment: IAttachment) {
}
}
.is-task-cover {
background: var(--primary);
color: var(--white);
padding: .25rem .35rem;
border-radius: 4px;
font-size: .75rem;
}
@include modal-transition();
</style>

View File

@ -11,62 +11,70 @@
@click.ctrl="() => toggleTaskDone(task)"
@click.meta="() => toggleTaskDone(task)"
>
<span class="task-id">
<Done class="kanban-card__done" :is-done="task.done" variant="small"/>
<template v-if="task.identifier === ''">
#{{ task.index }}
</template>
<template v-else>
{{ task.identifier }}
</template>
</span>
<span
:class="{'overdue': task.dueDate <= new Date() && !task.done}"
class="due-date"
v-if="task.dueDate > 0"
v-tooltip="formatDateLong(task.dueDate)">
<span class="icon">
<icon :icon="['far', 'calendar-alt']"/>
<img
v-if="coverImageBlobUrl"
:src="coverImageBlobUrl"
alt=""
class="cover-image"
/>
<div class="p-2">
dpschen marked this conversation as resolved Outdated

Why the extra wrapper instead of adding p-2 to the task-id?

Why the extra wrapper instead of adding `p-2` to the task-id?

Because it's not only the task id but every other element that's coming after that which would require the spacing. Adding a wrapper seemed simpler.

Before I added the cover image element the padding was on the card element itself.

Because it's not only the task id but every other element that's coming after that which would require the spacing. Adding a wrapper seemed simpler. Before I added the cover image element the padding was on the card element itself.

Oversaw the other elements. Makes sense!

Oversaw the other elements. Makes sense!
<span class="task-id">
<Done class="kanban-card__done" :is-done="task.done" variant="small"/>
<template v-if="task.identifier === ''">
#{{ task.index }}
</template>
<template v-else>
{{ task.identifier }}
</template>
</span>
<time :datetime="formatISO(task.dueDate)">
{{ formatDateSince(task.dueDate) }}
</time>
</span>
<h3>{{ task.title }}</h3>
<progress
class="progress is-small"
v-if="task.percentDone > 0"
:value="task.percentDone * 100" max="100">
{{ task.percentDone * 100 }}%
</progress>
<div class="footer">
<labels :labels="task.labels"/>
<priority-label :priority="task.priority" :done="task.done"/>
<div class="assignees" v-if="task.assignees.length > 0">
<user
v-for="u in task.assignees"
:avatar-size="24"
:key="task.id + 'assignee' + u.id"
:show-username="false"
:user="u"
/>
<span
:class="{'overdue': task.dueDate <= new Date() && !task.done}"
class="due-date"
v-if="task.dueDate > 0"
v-tooltip="formatDateLong(task.dueDate)">
<span class="icon">
<icon :icon="['far', 'calendar-alt']"/>
</span>
<time :datetime="formatISO(task.dueDate)">
{{ formatDateSince(task.dueDate) }}
</time>
</span>
<h3>{{ task.title }}</h3>
<progress
class="progress is-small"
v-if="task.percentDone > 0"
:value="task.percentDone * 100" max="100">
{{ task.percentDone * 100 }}%
</progress>
<div class="footer">
<labels :labels="task.labels"/>
<priority-label :priority="task.priority" :done="task.done"/>
<div class="assignees" v-if="task.assignees.length > 0">
<user
v-for="u in task.assignees"
:avatar-size="24"
:key="task.id + 'assignee' + u.id"
:show-username="false"
:user="u"
/>
</div>
<checklist-summary :task="task"/>
<span class="icon" v-if="task.attachments.length > 0">
<icon icon="paperclip"/>
</span>
<span v-if="task.description" class="icon">
<icon icon="align-left"/>
</span>
<span class="icon" v-if="task.repeatAfter.amount > 0">
<icon icon="history"/>
</span>
</div>
<checklist-summary :task="task"/>
<span class="icon" v-if="task.attachments.length > 0">
<icon icon="paperclip"/>
</span>
<span v-if="task.description" class="icon">
<icon icon="align-left"/>
</span>
<span class="icon" v-if="task.repeatAfter.amount > 0">
<icon icon="history"/>
</span>
</div>
</div>
</template>
<script lang="ts" setup>
import {ref, computed} from 'vue'
import {ref, computed, watch} from 'vue'
import {useRouter} from 'vue-router'
import PriorityLabel from '@/components/tasks/partials/priorityLabel.vue'
@ -77,6 +85,8 @@ import ChecklistSummary from './checklist-summary.vue'
import {TASK_DEFAULT_COLOR, getHexColor} from '@/models/task'
import type {ITask} from '@/modelTypes/ITask'
import {SUPPORTED_IMAGE_SUFFIX} from '@/models/attachment'
import AttachmentService from '@/services/attachment'
import {formatDateLong, formatISO, formatDateSince} from '@/helpers/time/formatDate'
import {colorIsDark} from '@/helpers/color/colorIsDark'
@ -114,6 +124,29 @@ function openTaskDetail() {
state: {backdropView: router.currentRoute.value.fullPath},
dpschen marked this conversation as resolved
Review

We should use here a router-link instead. This way we also support right click => 'open in new tab' and middle click.

We should use here a router-link instead. This way we also support right click => 'open in new tab' and middle click.
Review

I tried that, but it has weired side effects like styling (easily fixable) and not realiably opening the task detail view on click (not sure why, probably related to the drag and drop stuff). Let's leave this for another PR, I feel like this is kind of out of scope for this one.

I tried that, but it has weired side effects like styling (easily fixable) and not realiably opening the task detail view on click (not sure why, probably related to the drag and drop stuff). Let's leave this for another PR, I feel like this is kind of out of scope for this one.
Review

Makes sense!

Makes sense!
})
}
const coverImageBlobUrl = ref<string | null>(null)
async function maybeDownloadCoverImage() {
if (!props.task.coverImageAttachmentId) {
coverImageBlobUrl.value = null
return
}
const attachment = props.task.attachments.find(a => a.id === props.task.coverImageAttachmentId)
if (!attachment || !SUPPORTED_IMAGE_SUFFIX.some((suffix) => attachment.file.name.endsWith(suffix))) {
return
}
const attachmentService = new AttachmentService()
coverImageBlobUrl.value = await attachmentService.getBlobUrl(attachment)
}
watch(
() => props.task.coverImageAttachmentId,
maybeDownloadCoverImage,
{immediate: true},
)
</script>
<style lang="scss" scoped>
@ -125,12 +158,11 @@ $task-background: var(--white);
cursor: pointer;
box-shadow: var(--shadow-xs);
display: block;
border: 3px solid transparent;
font-size: .9rem;
padding: .4rem;
border-radius: $radius;
background: $task-background;
overflow: hidden;
&.loader-container.is-loading::after {
width: 1.5rem;

View File

@ -693,7 +693,11 @@
"deleteTooltip": "Delete this attachment",
"deleteText1": "Are you sure you want to delete the attachment {filename}?",
"copyUrl": "Copy URL",
"copyUrlTooltip": "Copy the url of this attachment for usage in text"
"copyUrlTooltip": "Copy the url of this attachment for usage in text",
"setAsCover": "Make cover",
konrad marked this conversation as resolved Outdated

I like the short version of https://github.com/go-vikunja/frontend/issues/53#issuecomment-1139502638

"Make cover" – "Remove cover"

'Unset' sounds so bulky

I like the short version of https://github.com/go-vikunja/frontend/issues/53#issuecomment-1139502638 "Make cover" – "Remove cover" 'Unset' sounds so bulky

Done.

Done.
"unsetAsCover": "Remove cover",
"successfullyChangedCoverImage": "The cover image was successfully changed.",
"usedAsCover": "Cover image"
},
"comment": {
"title": "Comments",

View File

@ -1,4 +1,4 @@
import type { Priority } from '@/constants/priorities'
import type {Priority} from '@/constants/priorities'
import type {IAbstract} from './IAbstract'
import type {IUser} from './IUser'
@ -11,6 +11,7 @@ import type {IBucket} from './IBucket'
import type {IRelationKind} from '@/types/IRelationKind'
import type {IRepeatAfter} from '@/types/IRepeatAfter'
import type {IRepeatMode} from '@/types/IRepeatMode'
export interface ITask extends IAbstract {
id: number
title: string
@ -31,8 +32,9 @@ export interface ITask extends IAbstract {
parentTaskId: ITask['id']
hexColor: string
percentDone: number
relatedTasks: Partial<Record<IRelationKind, ITask[]>>,
relatedTasks: Partial<Record<IRelationKind, ITask[]>>
attachments: IAttachment[]
coverImageAttachmentId: IAttachment['id']
identifier: string
index: number
isFavorite: boolean

View File

@ -5,6 +5,8 @@ import type { IUser } from '@/modelTypes/IUser'
import type { IFile } from '@/modelTypes/IFile'
import type { IAttachment } from '@/modelTypes/IAttachment'
export const SUPPORTED_IMAGE_SUFFIX = ['.jpg', '.png', '.bmp', '.gif']
dpschen marked this conversation as resolved Outdated

Align this with SUPPORTED_SUFFIX in attachments.vue.

Align this with `SUPPORTED_SUFFIX` in `attachments.vue`.

But that does not exist in the vue component anymore?

But that does not exist in the vue component anymore?

🤣
I searched locally and didn't think of that you obviously moved this from one file to another…

🤣 I searched locally and didn't think of that you obviously moved this from one file to another…
export default class AttachmentModel extends AbstractModel<IAttachment> implements IAttachment {
id = 0
taskId = 0

View File

@ -409,6 +409,13 @@ export const useTaskStore = defineStore('task', {
cancel()
}
},
async setCoverImage(task: ITask, attachment: IAttachment | null) {
return this.update({
...task,
coverImageAttachmentId: attachment ? attachment.id : 0,
})
},
},
})

View File

@ -218,7 +218,8 @@
<div class="content attachments" v-if="activeFields.attachments || hasAttachments">
<attachments
:edit-enabled="canWrite"
:task-id="taskId"
:task="task"
dpschen marked this conversation as resolved Outdated

Use v-model for task

Use v-model for task

See my comment above

See my comment above
@task-changed="({coverImageAttachmentId}) => task.coverImageAttachmentId = coverImageAttachmentId"
ref="attachments"
/>
</div>
@ -500,7 +501,7 @@ const attachmentStore = useAttachmentStore()
const taskStore = useTaskStore()
const kanbanStore = useKanbanStore()
const task = reactive(new TaskModel())
const task = reactive<ITask>(new TaskModel())
useTitle(toRef(task, 'title'))
// We doubled the task color property here because verte does not have a real change property, leading