Add color indicators to task list #321

Merged
konrad merged 4 commits from Bouni/vikunja-frontend:task-list-color-bubbles into master 2020-12-10 12:06:50 +00:00
Contributor

This PR adds color bubbles to the task list to make it more easy to spot to which list they belong as requestd in #246

This PR adds color bubbles to the task list to make it more easy to spot to which list they belong as requestd in #246 ![](https://kolaente.dev/attachments/b30c2f46-a1b0-4f41-bb12-9380c434eac0)
Bouni added 2 commits 2020-12-09 10:29:36 +00:00
continuous-integration/drone/pr Build is passing Details
e6a0b2a5cd
fixed tabs
Bouni changed title from WIP: Add color indicators to task list to Add color indicators to task list 2020-12-09 13:11:17 +00:00
konrad requested changes 2020-12-09 18:23:54 +00:00
konrad left a comment
Owner

Hey, thanks for the PR!

Some small nits, but looks great overall.

Hey, thanks for the PR! Some small nits, but looks great overall.
@ -2,2 +2,4 @@
<div :class="{'is-loading': taskService.loading}" class="task loader-container">
<fancycheckbox :disabled="isArchived || disabled" @change="markAsDone" v-model="task.done"/>
<span
:style="{backgroundColor: $store.getters['lists/getListById'](task.listId).hexColor }"

Could you put this in a computed property?

Could you put this in a computed property?
Bouni marked this conversation as resolved
@ -53,6 +53,14 @@
display: none;
}
.color-bubble {

Could you move the general styling (everything but the sizing) from theme/navigation.scss to theme/theme.scss and then just override the size here?

Could you move the general styling (everything but the sizing) from `theme/navigation.scss` to `theme/theme.scss` and then just override the size here?
Bouni marked this conversation as resolved
Author
Contributor

Hi,

I tried to put the listColor into computed as you suggested, it works but I see a lot of errors in the console!?

<span
    :style="{backgroundColor: listColor(task.listId) }"
    class="color-bubble"
    v-if="listColor(task.listId) !== ''">
</span>
 computed: {
     listColor() {
         return listId => this.$store.getters['lists/getListById'](listId).hexColor || "" 
     },
[Vue warn]: Error in render: "TypeError: _this.$store.getters['lists/getListById'](...) is null"

found in

---> <SingleTaskInList> at src/components/tasks/partials/singleTaskInList.vue
       <ShowTasks> at src/views/tasks/ShowTasks.vue
         <Home> at src/views/Home.vue
           <ContentAuth> at src/components/home/contentAuth.vue
             <App> at src/App.vue
               <Root>

Any ideas why this happens?

Hi, I tried to put the listColor into computed as you suggested, it works but I see a lot of errors in the console!? ``` <span :style="{backgroundColor: listColor(task.listId) }" class="color-bubble" v-if="listColor(task.listId) !== ''"> </span> ``` ```js computed: { listColor() { return listId => this.$store.getters['lists/getListById'](listId).hexColor || "" }, ``` ``` [Vue warn]: Error in render: "TypeError: _this.$store.getters['lists/getListById'](...) is null" found in ---> <SingleTaskInList> at src/components/tasks/partials/singleTaskInList.vue <ShowTasks> at src/views/tasks/ShowTasks.vue <Home> at src/views/Home.vue <ContentAuth> at src/components/home/contentAuth.vue <App> at src/App.vue <Root> ``` Any ideas why this happens?
Bouni added 1 commit 2020-12-10 06:38:19 +00:00
Author
Contributor

I figured it out, at first the getter is called with listId 0 which returns null 🤷‍♂️

I figured it out, at first the getter is called with listId 0 which returns null 🤷‍♂️
Bouni requested review from konrad 2020-12-10 07:17:56 +00:00
konrad requested changes 2020-12-10 09:52:47 +00:00
@ -122,2 +127,4 @@
},
computed: {
listColor() {
return listId => listId > 0 ? this.$store.getters['lists/getListById'](listId).hexColor : ""

Since this component only ever has one task, could you change this to include the task directly? Instead of passing it as an argument.
It would also be great if you would check if the list is null instead of checking if the listId is > 0 - imagine a bad list id which is > 0, that would still yield an error to the console.

TL;DR:

listColor() {
	const list = this.$store.getters['lists/getListById'](this.task.listId)
    return list !== null ? list.hexColor : null
}
Since this component only ever has one task, could you change this to include the task directly? Instead of passing it as an argument. It would also be great if you would check if the list is null instead of checking if the `listId` is > 0 - imagine a bad list id which is > 0, that would still yield an error to the console. TL;DR: ``` listColor() { const list = this.$store.getters['lists/getListById'](this.task.listId) return list !== null ? list.hexColor : null } ```

The last part should return '' if the list is null, my bad.

The last part should return `''` if the list is null, my bad.
Bouni marked this conversation as resolved
Bouni added 1 commit 2020-12-10 11:18:07 +00:00
continuous-integration/drone/pr Build is passing Details
9a206319f7
fix requested changes
Bouni requested review from konrad 2020-12-10 11:18:55 +00:00
konrad approved these changes 2020-12-10 12:06:36 +00:00
konrad left a comment
Owner

Thanks!

Thanks!
konrad merged commit 1dcf4520a0 into master 2020-12-10 12:06:50 +00:00
Bouni deleted branch task-list-color-bubbles 2021-01-11 16:32:01 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.