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
2 changed files with 60 additions and 54 deletions

View File

@ -11,7 +11,7 @@
</x-button>
</div>
<filter-popup
@change="loadTasks"
@change="loadTasks()"
konrad marked this conversation as resolved Outdated

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.
:visible="showTaskFilter"
v-model="params"
/>

View File

@ -1,4 +1,4 @@
import TaskCollectionService from '../../../services/taskCollection'
import TaskCollectionService from '@/services/taskCollection'
import cloneDeep from 'lodash/cloneDeep'
import {calculateItemPosition} from '../../../helpers/calculateItemPosition'
konrad marked this conversation as resolved Outdated

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.
@ -12,16 +12,59 @@ const DEFAULT_PARAMS = {
filter_concat: 'and',
}
function createPagination(totalPages, currentPage) {
const pages = []
for (let i = 0; i < totalPages; i++) {
// Show ellipsis instead of all pages
if (
i > 0 && // Always at least the first page
(i + 1) < totalPages && // And the last page
(
// And the current with current + 1 and current - 1
(i + 1) > currentPage + 1 ||
(i + 1) < currentPage - 1
)
) {
// Only add an ellipsis if the last page isn't already one
if (pages[i - 1] && !pages[i - 1].isEllipsis) {
pages.push({
number: 0,
isEllipsis: true,
})
}
continue
}
pages.push({
number: i + 1,
isEllipsis: false,
})
}
return pages
}
export function getRouteForPagination(page = 1, type = 'list') {
return {
name: 'list.' + type,
params: {
type: type,
},
query: {
page: page,
},
}
}
/**
* This mixin provides a base set of methods and properties to get tasks on a list.
*/
export default {
data() {
return {
taskCollectionService: TaskCollectionService,
taskCollectionService: new TaskCollectionService(),
tasks: [],
pages: [],
currentPage: 0,
loadedList: null,
@ -34,16 +77,17 @@ export default {
}
},
watch: {
konrad marked this conversation as resolved Outdated

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.
'$route.query': 'loadTasksForPage', // Only listen for query path changes
// Only listen for query path changes
'$route.query': {
handler: 'loadTasksForPage',
immediate: true,
},
konrad marked this conversation as resolved Outdated

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 :)
'$route.path': 'loadTasksOnSavedFilter',
},
beforeMount() {
// Triggering loading the tasks in beforeMount lets the component maintain the current page, therefore the page
// is not lost after navigating back from a task detail page for example.
this.loadTasksForPage(this.$route.query)
},
created() {
this.taskCollectionService = new TaskCollectionService()
computed: {
pages() {
return createPagination(this.taskCollectionService.totalPages, this.currentPage)
},
},
methods: {
loadTasks(
@ -83,48 +127,20 @@ export default {
return
}
this.$set(this, 'tasks', [])
this.tasks = []
this.taskCollectionService.getAll(list, params, page)
konrad marked this conversation as resolved Outdated

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.
.then(r => {
this.$set(this, 'tasks', r)
this.$set(this, 'pages', [])
this.tasks = r
this.currentPage = page
for (let i = 0; i < this.taskCollectionService.totalPages; i++) {
// Show ellipsis instead of all pages
if (
i > 0 && // Always at least the first page
(i + 1) < this.taskCollectionService.totalPages && // And the last page
(
// And the current with current + 1 and current - 1
(i + 1) > this.currentPage + 1 ||
(i + 1) < this.currentPage - 1
)
) {
// Only add an ellipsis if the last page isn't already one
if (this.pages[i - 1] && !this.pages[i - 1].isEllipsis) {
this.pages.push({
number: 0,
isEllipsis: true,
})
}
continue
}
this.pages.push({
number: i + 1,
isEllipsis: false,
})
}
this.loadedList = cloneDeep(currentList)
})
.catch(e => {
this.error(e)
})
},
loadTasksForPage(e) {
// The page parameter can be undefined, in the case where the user loads a new list from the side bar menu
let page = Number(e.page)
@ -180,17 +196,6 @@ export default {
this.showTaskSearch = false
}, 200)
},
getRouteForPagination(page = 1, type = 'list') {
return {
name: 'list.' + type,
params: {
type: type,
},
query: {
page: page,
},
}
},
saveTaskPosition(e) {
this.drag = false
@ -208,5 +213,6 @@ export default {
this.error(e)
})
},
getRouteForPagination,
},
}