feat simplify taskList mixin #728

Merged
konrad merged 2 commits from dpschen/frontend:feature/simplify-taskList-mixin into main 2021-09-10 14:39:18 +00:00
Member
No description provided.
dpschen reviewed 2021-09-09 14:28:59 +00:00
@ -12,3 +12,3 @@
</div>
<filter-popup
@change="loadTasks"
@change="loadTasks()"
Author
Member

Keeping the parameters of the filter-popup change event in sync with the loadTasks parameters if error prone.
I'm not sure if it was intended here to provide the parameters (didn't check filterPopup).

Keeping the parameters of the filter-popup change event in sync with the loadTasks parameters if error prone. I'm not sure if it was intended here to provide the parameters (didn't check filterPopup).

AFAIK the event parameters are not used, I was just lazy when I added the change listener here.

AFAIK the event parameters are not used, I was just lazy when I added the change listener here.
konrad marked this conversation as resolved
dpschen reviewed 2021-09-09 14:29:12 +00:00
dpschen reviewed 2021-09-09 14:30:33 +00:00
@ -2,2 +1,3 @@
import TaskCollectionService from '@/services/taskCollection'
import cloneDeep from 'lodash/cloneDeep'
import {calculateItemPosition} from '../../../helpers/calculateItemPosition'
Author
Member

Since the following two functions just calculate some result based on input I removed the definition of them from the mixin in preparation for future composition api.

Since the following two functions just calculate some result based on input I removed the definition of them from the mixin in preparation for future composition api.
konrad marked this conversation as resolved
dpschen reviewed 2021-09-09 14:31:41 +00:00
@ -35,0 +76,4 @@
// Only listen for query path changes
'$route.query': {
handler: 'loadTasksForPage',
immediate: true,
Author
Member

With immediate: true the watcher is called on creation -> no additional call necessary in beforeMount, created, etc.

With `immediate: true` the watcher is called on creation -> no additional call necessary in `beforeMount`, `created`, etc.

Ohhh that's a really nice feature I didn't know.

Ohhh that's a really nice feature I didn't know.
konrad marked this conversation as resolved
dpschen reviewed 2021-09-09 14:32:31 +00:00
@ -39,2 +83,2 @@
// is not lost after navigating back from a task detail page for example.
this.loadTasksForPage(this.$route.query)
computed: {
pages() {
Author
Member

By using a computed for pages we can spare us the hassle of keeping pages manually in sync and just let the automatic watcher do their job :)

By using a computed for `pages` we can spare us the hassle of keeping pages manually in sync and just let the automatic watcher do their job :)
konrad marked this conversation as resolved
dpschen reviewed 2021-09-09 14:33:56 +00:00
@ -51,1 +92,3 @@
) {
loadTasks(givenParams) {
const defaultParams = {
page: 1,
Author
Member

Because a page of 1 is the default I could remove the parameter 1 in all the other calls. Need to check if that makes sense.

Because a page of `1` is the default I could remove the parameter `1` in all the other calls. Need to check if that makes sense.
Author
Member

Removed aswell.

Removed aswell.
dpschen marked this conversation as resolved
dpschen reviewed 2021-09-09 14:35:33 +00:00
@ -60,10 +114,6 @@ export default {
return
}
Author
Member

Tbh I'm not sure if removing the following lines is correct. Don't remember why I did.

Tbh I'm not sure if removing the following lines is correct. Don't remember why I did.
Author
Member

Was probably wrong. Removed this

Was probably wrong. Removed this
dpschen marked this conversation as resolved
dpschen reviewed 2021-09-09 14:37:56 +00:00
@ -80,58 +130,24 @@ export default {
return
}
Author
Member

In Vue 2 the $set function is just needed the for change detection caveats. Directly replacing a reactive data value is not one of those. So no $set needed (we don't need it anywhere in vue 3).

In Vue 2 the `$set` function is just needed the for [change detection caveats](https://vuejs.org/v2/guide/reactivity.html#Change-Detection-Caveats). Directly replacing a reactive data value is not one of those. So no `$set` needed (we don't need it anywhere in vue 3).

I see, I thought it was required for all things array mutations, not only directly replacing a value in an array.

I see, I thought it was required for all things array mutations, not only directly replacing a value in an array.
konrad marked this conversation as resolved
dpschen reviewed 2021-09-09 14:44:35 +00:00
@ -132,3 +149,1 @@
if (typeof e.search === 'undefined') {
search = ''
}
const page = e.page !== 'undefined' && Number(e.page)
Author
Member

We just assign the params if they have a valid value. In other cases the default their default value is used in the initTasks function

We just assign the params if they have a valid value. In other cases the default their default value is used in the `initTasks` function
Author
Member

Probably also wrong. Removed.

Probably also wrong. Removed.
dpschen marked this conversation as resolved
dpschen force-pushed feature/simplify-taskList-mixin from 10948a1f62 to e0fe45cceb 2021-09-09 14:51:17 +00:00 Compare
dpschen force-pushed feature/simplify-taskList-mixin from e0fe45cceb to a48f814f56 2021-09-09 15:28:40 +00:00 Compare
dpschen changed title from WIP: feat simplify taskList mixin to feat simplify taskList mixin 2021-09-09 15:48:48 +00:00
konrad approved these changes 2021-09-10 14:15:37 +00:00
konrad added 1 commit 2021-09-10 14:21:16 +00:00
continuous-integration/drone/pr Build is passing Details
7a4b5d6449
Merge branch 'main' into feature/simplify-taskList-mixin
# Conflicts:
#	src/components/tasks/mixins/taskList.js
konrad merged commit 50c1a2e4d5 into main 2021-09-10 14:39:18 +00:00
konrad deleted branch feature/simplify-taskList-mixin 2021-09-10 14:39:18 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.