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
Owner

This PR implements setting an uploaded attachment as cover image for a task which will make it show up on the kanban board. Implementing this with attachments allowed to re-use a bunch of existing logic around upload, permissions and retrieval of the actual image file.

Depends on #2459

Frontend PR for vikunja/api#1263

Resolves https://github.com/go-vikunja/frontend/issues/53
Related forum discussion: https://community.vikunja.io/t/feature-request-use-image-attachments-as-cover-image-on-kanban-cards-like-trello/351/2

This PR implements setting an uploaded attachment as cover image for a task which will make it show up on the kanban board. Implementing this with attachments allowed to re-use a bunch of existing logic around upload, permissions and retrieval of the actual image file. Depends on https://kolaente.dev/vikunja/frontend/pulls/2459 Frontend PR for https://kolaente.dev/vikunja/api/pulls/1263 Resolves https://github.com/go-vikunja/frontend/issues/53 Related forum discussion: https://community.vikunja.io/t/feature-request-use-image-attachments-as-cover-image-on-kanban-cards-like-trello/351/2
konrad added 1 commit 2022-10-02 11:44:27 +00:00
continuous-integration/drone/pr Build is passing Details
a5925baff0
feat: migrate kanban card to script setup
konrad requested review from dpschen 2022-10-02 11:44:39 +00:00
Member

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://2460-feature-task-cover--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi konrad! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://2460-feature-task-cover--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
konrad changed title from WIP: feat(task): cover image for tasks to feat(task): cover image for tasks 2022-10-03 10:41:19 +00:00
dpschen reviewed 2022-10-03 11:26:27 +00:00
@ -81,0 +83,4 @@
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') }}

Picky:
line wrap

Picky: line wrap
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
@ -170,0 +166,4 @@
import {useTaskStore} from '@/stores/tasks'
import {useI18n} from 'vue-i18n'
const taskStore = useTaskStore()

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?
@ -170,0 +167,4 @@
import {useI18n} from 'vue-i18n'
const taskStore = useTaskStore()
const {t} = useI18n()

add global scope

add global scope
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
@ -170,2 +177,4 @@
editEnabled: true,
})
// FIXME: this should go through the store

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`?
Author
Owner

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?
Author
Owner

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.
@ -172,1 +180,4 @@
// FIXME: this should go through the store
const emit = defineEmits(['task-changed'])
const attachmentService = shallowReactive(new AttachmentService())

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.
konrad marked this conversation as resolved
@ -239,0 +252,4 @@
copy(generateAttachmentUrl(props.task.id, attachment.id))
}
async function setCoverImage(attachment: IAttachment | null) {

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.
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
@ -30,0 +17,4 @@
alt=""
class="cover-image"
/>
<div class="p-2">

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?
Author
Owner

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!
dpschen marked this conversation as resolved
@ -142,0 +121,4 @@
}
}
function openTaskDetail() {

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.
Author
Owner

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.

Makes sense!

Makes sense!
dpschen marked this conversation as resolved
@ -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']

Align this with SUPPORTED_SUFFIX in attachments.vue.

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

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…
dpschen marked this conversation as resolved
@ -219,3 +219,3 @@
<attachments
:edit-enabled="canWrite"
:task-id="taskId"
:task="task"

Use v-model for task

Use v-model for task
Author
Owner

See my comment above

See my comment above
dpschen marked this conversation as resolved
konrad was assigned by dpschen 2022-10-03 11:26:34 +00:00
konrad force-pushed feature/task-cover from a491d418ac to 0ca6588139 2022-10-03 13:26:05 +00:00 Compare
dpschen approved these changes 2022-10-03 15:57:05 +00:00
@ -78,6 +78,17 @@
>
{{ $t('misc.delete') }}
</BaseButton>
<BaseButton

I feel like only changing the wording of the button isn't enough indication for the user which cover image is currently set, if he has the task detail open. When you change it you get a notification which is fine. But if you have a list of attachments you would need to scroll through them and check the wording of the buttons in order to see the current state.

I'm not sure what a good solution could be here. Maybe something like the icon used in the GitHub tread: https://github.com/go-vikunja/frontend/issues/53#issuecomment-1139502638.
I think only the current image should have the icon though, in order to have less visual pollution.
If all attachments would have the icon it would be hard to scan again.

Open for alternatives

I feel like only changing the wording of the button isn't enough indication for the user which cover image is currently set, if he has the task detail open. When you change it you get a notification which is fine. But if you have a list of attachments you would need to scroll through them and check the wording of the buttons in order to see the current state. I'm not sure what a good solution could be here. Maybe something like the icon used in the GitHub tread: https://github.com/go-vikunja/frontend/issues/53#issuecomment-1139502638. I think only the current image should have the icon though, in order to have less visual pollution. If all attachments would have the icon it would be hard to scan again. Open for alternatives
Author
Owner

Maybe even add a little badge (like the "done" we have for done tasks on the kanban board) next to the attachment that's used as cover?

Maybe even add a little badge (like the "done" we have for done tasks on the kanban board) next to the attachment that's used as cover?

I like that. Could also be like the pills used for selected users in the multiselect. this way it could also have an 'x' to remove it again.

I like that. Could also be like the pills used for selected users in the multiselect. this way it could also have an 'x' to remove it again.
Author
Owner

I've now added this:

I've now added this: ![](https://kolaente.dev/attachments/07622ba0-9c42-4e0c-9a82-da5b23baea0f)

Looks good! :)

Looks good! :)
dpschen marked this conversation as resolved
dpschen reviewed 2022-10-03 16:02:19 +00:00
@ -695,2 +695,3 @@
"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": "Set as cover image",

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
Author
Owner

Done.

Done.
konrad marked this conversation as resolved
konrad force-pushed feature/task-cover from 1038d7b764 to a7731370a0 2022-10-05 12:56:27 +00:00 Compare
konrad added 1 commit 2022-10-05 13:04:40 +00:00
continuous-integration/drone/pr Build is passing Details
f01107fd73
feat: add indicator if an attachment is task cover
Author
Owner
No description provided.
konrad merged commit 31e39aa6c8 into main 2022-10-05 13:28:13 +00:00
konrad deleted branch feature/task-cover 2022-10-05 13:28:13 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.