feat: show namespace of related tasks if they are different than the current one #923

konrad · 2021-10-31T13:35:08Z

konrad commented

2021-10-31 13:35:08 +00:00

Resolves #588

konrad requested review from dpschen 2021-10-31 13:39:21 +00:00

dpschen requested changes 2021-10-31 13:59:09 +00:00

src/components/tasks/partials/relatedTasks.vue Outdated

						
				@ -45,1 +44,3 @@

													{{ $store.getters['lists/getListById'](props.option.listId) === null ? '' : $store.getters['lists/getListById'](props.option.listId).title }} >

												>

													<span

														v-if="$store.getters['namespaces/getListAndNamespaceById'](props.option.listId) !== null && $store.getters['namespaces/getListAndNamespaceById'](props.option.listId).namespace.id !== namespace.id"

dpschen commented

2021-10-31 13:54:55 +00:00

This should be a computed that returns a function.
Since namespace.id should never be null (?) I think the first condition can be removed ($store.getters['namespaces/getListAndNamespaceById'](props.option.listId) !== null )

- This should be a computed that returns a function. - Since namespace.id should never be `null` (?) I think the first condition can be removed (`$store.getters['namespaces/getListAndNamespaceById'](props.option.listId) !== null `)

konrad commented

2021-10-31 14:18:26 +00:00

This should be a computed that returns a function.

Done.

Since namespace.id should never be null (?) I think the first condition can be removed ($store.getters'namespaces/getListAndNamespaceById' !== null )

Theoretically, it could be null. Even though very unlikely.

> This should be a computed that returns a function. Done. > Since namespace.id should never be null (?) I think the first condition can be removed ($store.getters['namespaces/getListAndNamespaceById'](props.option.listId) !== null ) Theoretically, it could be `null`. Even though very unlikely.

konrad marked this conversation as resolved

src/components/tasks/partials/relatedTasks.vue Outdated

						
				@ -86,1 +94,3 @@

													}} >

												>

													<span

														v-if="$store.getters['namespaces/getListAndNamespaceById'](t.listId) !== null && $store.getters['namespaces/getListAndNamespaceById'](t.listId).namespace.id !== namespace.id"

dpschen commented

2021-10-31 13:58:48 +00:00

relatedTasks (where t.listId is indirectly coming from) should be a computed so that this computation doesn't need to be inline and can be cached.

`relatedTasks` (where `t.listId` is indirectly coming from) should be a computed so that this computation doesn't need to be inline and can be cached.

konrad commented

2021-10-31 14:18:59 +00:00

Done as well.

dpschen commented

2021-10-31 23:13:40 +00:00

I meant something similar to this to reduce duplicate call to the store getter functions:

computed: {
    mappedRelatedTasks() {
        function mapRelatedTasks(tasks) {
            return tasks
                // FIXME: Why is the filter necessary? shouldn't all tasks be `true`?
                .filter(t => t)
                .map((task) => {
                    // by doing this here once we can save a lot of duplicate calls in the template
                    const {list, namespace} = this.$store.getters['namespaces/getListAndNamespaceById'](task.listId)
                    return {
                        ...task,
                        differntNamespace:
                            namespace !== null &&
                            namespace.id !== this.namespace.id &&
                            namespace?.title || ''
                        differntList:
                            list !== null &&
                            task.listId !== this.listId &&
                            list?.title || ''
                    }
                })
        },

        return this.relatedTasks.map((rts, kind) => ({
            title: this.$tc(`task.relation.kinds.${kind}`, rts.length)
            tasks: mapRelatedTasks(rts)
        }))
    }
}

I meant something similar to this to reduce duplicate call to the store getter functions: ```js computed: { mappedRelatedTasks() { function mapRelatedTasks(tasks) { return tasks // FIXME: Why is the filter necessary? shouldn't all tasks be `true`? .filter(t => t) .map((task) => { // by doing this here once we can save a lot of duplicate calls in the template const {list, namespace} = this.$store.getters['namespaces/getListAndNamespaceById'](task.listId) return { ...task, differntNamespace: namespace !== null && namespace.id !== this.namespace.id && namespace?.title || '' differntList: list !== null && task.listId !== this.listId && list?.title || '' } }) }, return this.relatedTasks.map((rts, kind) => ({ title: this.$tc(`task.relation.kinds.${kind}`, rts.length) tasks: mapRelatedTasks(rts) })) } } ```

konrad commented

2021-11-02 18:53:26 +00:00

Ah yes, that makes a lot of sense! I've changed it accordingly.

konrad marked this conversation as resolved

konrad requested review from dpschen 2021-10-31 15:24:27 +00:00

dpschen requested changes 2021-11-01 00:26:37 +00:00

src/components/tasks/partials/relatedTasks.vue Outdated

						
				@ -46,0 +51,4 @@

													</span>

													<span v-tooltip="$t('task.relation.differentList')">

														{{

															$store.getters['lists/getListById'](props.option.listId) === null ? '' : $store.getters['lists/getListById'](props.option.listId).title

dpschen commented

2021-10-31 22:15:16 +00:00

$store.getters['lists/getListById'](props.option.listId)?.title || ''

```js $store.getters['lists/getListById'](props.option.listId)?.title || '' ```

konrad marked this conversation as resolved

src/components/tasks/partials/relatedTasks.vue Outdated

						
				@ -87,0 +101,4 @@

													</span>

													<span v-tooltip="$t('task.relation.differentList')">

														{{

															$store.getters['lists/getListById'](t.listId) === null ? '' : $store.getters['lists/getListById'](t.listId).title

dpschen commented

2021-10-31 22:16:20 +00:00

$store.getters['lists/getListById'](t.listId)?.title || ''

```js $store.getters['lists/getListById'](t.listId)?.title || '' ```

konrad commented

2021-11-02 18:30:59 +00:00

Done.

konrad marked this conversation as resolved

dpschen self-assigned this 2021-11-01 00:40:26 +00:00

konrad was assigned by dpschen

2021-11-01 00:40:43 +00:00

dpschen removed their assignment 2021-11-01 00:40:52 +00:00

konrad force-pushed feature/show-namespace-when-searching from 11a1167a83 to 4e913af56d

2021-11-02 18:52:56 +00:00

Compare

konrad requested review from dpschen 2021-11-02 18:53:05 +00:00

konrad added 1 commit 2021-11-02 18:57:23 +00:00

continuous-integration/drone/pr Build is passing Details

5ae4809a44

Merge branch 'main' into feature/show-namespace-when-searching

dpschen reviewed 2021-11-02 19:05:50 +00:00

src/components/tasks/partials/relatedTasks.vue Outdated

						
				@ -77,3 +82,1 @@

									<div :key="t.id" class="task" v-for="t in rts.filter(t => t)">

										<router-link :to="{ name: $route.name, params: { id: t.id } }">

											<span :class="{ 'done': t.done}" class="tasktext">

						<div :key="kind" class="related-tasks" v-for="(rts, kind ) in mappedRelatedTasks">

dpschen commented

2021-11-02 19:05:50 +00:00

I think the :key is old.

I think the `:key` is old.

konrad commented

2021-11-02 19:19:12 +00:00

It kind of is, but we do need it to remove a task. I've added it back in and made sure it is correctly used.

konrad marked this conversation as resolved

dpschen reviewed 2021-11-02 19:07:36 +00:00

src/components/tasks/partials/relatedTasks.vue Outdated

						
				@ -186,0 +201,4 @@

							return this.$store.getters['namespaces/getListAndNamespaceById'](this.listId, true)?.namespace

						},

						mappedRelatedTasks() {

							return Object.keys(this.relatedTasks).map(kind => ({

dpschen commented

2021-11-02 19:07:36 +00:00

(optional): use Object.entries + spread the entry with [kind, task].

(optional): use `Object.entries` + spread the entry with `[kind, task]`.

konrad commented

2021-11-02 19:21:25 +00:00

Good idea. Done.

konrad marked this conversation as resolved

konrad added 2 commits 2021-11-02 19:18:41 +00:00

edc74fd598

fix: add kind to mapped related tasks

continuous-integration/drone/pr Build is passing Details

21ddd25714

fix: related task not removed from the list when deleting it

konrad added 1 commit 2021-11-02 19:21:16 +00:00

continuous-integration/drone/pr Build is passing Details

e702524b94

chore: use Object.entries

dpschen reviewed 2021-11-02 19:54:42 +00:00

src/components/tasks/partials/relatedTasks.vue Outdated

						
				@ -325,0 +365,4 @@

							border-radius: $radius;

							border: 2px solid transparent;

							a {

dpschen commented

2021-11-02 19:54:42 +00:00

just realized that tasktext (in css above) can me merged with this <a>. Or do I miss a reason for the <span>?

just realized that tasktext (in css above) can me merged with this `<a>`. Or do I miss a reason for the `<span>`?

konrad commented

2021-11-02 22:00:10 +00:00

No real reason. The <span> probably comes from copy-pasting from other places.

Changed and cleaned up.

No real reason. The `<span>` probably comes from copy-pasting from other places. Changed and cleaned up.

konrad marked this conversation as resolved

dpschen reviewed 2021-11-02 19:56:14 +00:00

src/components/tasks/partials/relatedTasks.vue Outdated

						
				@ -325,0 +374,4 @@

								}

							}

							.remove {

dpschen commented

2021-11-02 19:56:14 +00:00

how about just showing the remove icon when the task / maybe even the whole relatedtasks module is hovered?

konrad commented

2021-11-02 22:01:34 +00:00

I think that's a great idea! Added.

konrad marked this conversation as resolved

konrad added 1 commit 2021-11-02 21:59:36 +00:00

continuous-integration/drone/pr Build is passing Details

3e7edd84fa

chore: simplify task title markup

konrad added 1 commit 2021-11-02 22:01:24 +00:00

continuous-integration/drone/pr Build is passing Details

2933fdcd3d

feat: only show delete icons on hover

konrad removed their assignment 2021-11-03 19:45:29 +00:00

dpschen was assigned by konrad

2021-11-03 19:45:29 +00:00

dpschen reviewed 2021-11-03 20:10:46 +00:00

src/components/tasks/partials/relatedTasks.vue Outdated

						
				@ -321,0 +320,4 @@

					&.is-narrow .columns {

						display: block;

						.column {

dpschen commented

2021-11-03 20:10:46 +00:00

Un~~scope~~nest not bulma specfic styles

Un~~scope~~nest not bulma specfic styles

konrad commented

2021-11-03 20:46:34 +00:00

Actually, these styles are not used at all. I've removed them.

👍 1

dpschen commented

2021-11-04 12:05:28 +00:00

Removing unused styles is cool.

What I meant was more the other styles:
There is no need to nest

.different-list
.related-tasks
[...]

They don't have to be inside .task-relations, because we now have scoped styles.
This makes styling in general much easier to handle.

Removing unused styles is cool. What I meant was more the other styles: There is no need to nest - `.different-list` - `.related-tasks` - [...] They don't have to be inside `.task-relations`, because we now have scoped styles. This makes styling in general much easier to handle.

konrad commented

2021-11-09 19:02:42 +00:00

Ahhh yes, that makes a lot of sense. Will refactor it.

konrad commented

2021-11-09 22:55:33 +00:00

Done.

konrad marked this conversation as resolved

dpschen reviewed 2021-11-03 20:29:10 +00:00

src/components/tasks/partials/relatedTasks.vue Outdated

						
				@ -222,3 +245,1 @@

										this.relatedTasks[relationKind][t].id === this.relationToDelete.otherTaskId &&

										relationKind === this.relationToDelete.relationKind

									if (!found) return false

								outer:

dpschen commented

2021-11-03 20:29:10 +00:00

I think this is the first time I have encountered a label in the wild :D

Not a fan of this code — neither how it was — nor the new one.
Any idea how to simplify this in general?

I think this is the first time I have encountered a label in the wild :D Not a fan of this code — neither how it was — nor the new one. Any idea how to simplify this in general?

konrad commented

2021-11-03 20:55:32 +00:00

I think this is the first time I have encountered a label in the wild :D

And it's not even the first in Vikunja 🙃

Not a fan of this code — neither how it was — nor the new one.
Any idea how to simplify this in general?

I think I was able to refactor it quite a bit in 2f28b43243 - please take another look.

I'm not sure if there is a more javascript-y way of doing this, but the thing it was before wasn't properly tracked by vue and therefore the UI did not remove the related task after removing it from the array. This one works.

> I think this is the first time I have encountered a label in the wild :D And it's not even the first in Vikunja 🙃 > Not a fan of this code — neither how it was — nor the new one. Any idea how to simplify this in general? I think I was able to refactor it quite a bit in 2f28b43243500bceff02ddf02b626759a58c8417 - please take another look. I'm not sure if there is a more javascript-y way of doing this, but the thing it was before wasn't properly tracked by vue and therefore the UI did not remove the related task after removing it from the array. This one works.

dpschen commented

2021-11-04 11:58:27 +00:00

And it's not even the first in Vikunja 🙃

:D

I think I was able to refactor it quite a bit in 2f28b43243 - please take another look.

This looks nice!
Using Array.some like it was before and then not using the result (to have something similar to break) is also kind of a hack.

> And it's not even the first in Vikunja 🙃 :D > I think I was able to refactor it quite a bit in 2f28b43243 - please take another look. This looks nice! Using `Array.some` like it was before and then not using the result (to have something similar to `break`) is also kind of a hack.

dpschen marked this conversation as resolved

konrad added 1 commit 2021-11-03 20:46:21 +00:00

continuous-integration/drone/pr Build is passing Details

ce5de84c84

chore: remove unused styles

konrad added 1 commit 2021-11-03 20:53:31 +00:00

continuous-integration/drone/pr Build is passing Details

2f28b43243

chore: refactor removing a related task

dpschen reviewed 2021-11-04 12:07:03 +00:00

src/components/tasks/partials/relatedTasks.vue Outdated

						
				@ -291,2 +329,2 @@

				        width: calc(100% - #{$remove-icon-width});

				      }

							a:not(.remove) {

								width: calc(100% - #{$remove-icon-width});

dpschen commented

2021-11-04 12:07:03 +00:00

Why does the a need a width? Asking because it's display: flex;.

Wouldn't flex: 1; be enough?

Why does the `a` need a width? Asking because it's `display: flex;`. Wouldn't `flex: 1;` be enough?

konrad commented

2021-11-09 19:02:13 +00:00

Back in the day, when I didn't know how flexbox worked, I think this was done using float...

I think that was originally used to make sure the icon sticks at the side. I've refactored it to use flex's justify-content instead.

Back in the day, when I didn't know how flexbox worked, I think this was done using float... I think that was originally used to make sure the icon sticks at the side. I've refactored it to use flex's `justify-content` instead.

konrad marked this conversation as resolved

konrad added 1 commit 2021-11-09 19:01:26 +00:00

continuous-integration/drone/pr Build is passing Details

467ddf716f

fix: simplify task text width

konrad added 1 commit 2021-11-09 19:05:21 +00:00

continuous-integration/drone/pr Build is passing Details

833482a19e

chore: cleanup nesting

konrad commented

2021-11-09 22:57:48 +00:00

@dpschen Could you give this another review?

dpschen requested changes 2021-11-11 01:12:19 +00:00

dpschen left a comment

Rest looks good! =)

src/components/tasks/partials/relatedTasks.vue

						
				@ -327,1 +349,3 @@

				  }

				}

				.related-tasks:hover .tasks .task .remove {

dpschen commented

2021-11-11 01:11:03 +00:00

you removed the related-tasks class. Should probably go back in =)

you removed the `related-tasks` class. Should probably go back in =)

👍 1

konrad commented

2021-11-13 15:59:51 +00:00

Where did I remove that 🤔 The hover effect still works...

dpschen commented

2021-11-13 19:27:49 +00:00

You are right… My bad! I confused it for task-relations.
Nevertheless I would keep that class even if unused: makes it much easier to debug if the wrapper of the component has the same class name as the components name =) (better then would be of course related-tasks that's where my confusion came from).

You are right… My bad! I confused it for `task-relations`. Nevertheless I would keep that class even if unused: makes it much easier to debug if the wrapper of the component has the same class name as the components name =) (better then would be of course `related-tasks` that's where my confusion came from).

👍 1

konrad commented

2021-11-13 19:52:59 +00:00

Makes sense - added.

konrad marked this conversation as resolved

konrad added 1 commit 2021-11-13 15:59:57 +00:00

continuous-integration/drone/pr Build is passing Details

aeec4b144d

Merge branch 'main' into feature/show-namespace-when-searching

frederick commented

2021-11-13 16:06:56 +00:00

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://923-featureshow-namespace-when-searching--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://923-featureshow-namespace-when-searching--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 added 1 commit 2021-11-13 19:52:48 +00:00