feat/alphabetical-sort #1162

Merged
dpschen merged 9 commits from :feat/alphabetical-sort into main 2021-12-21 16:29:50 +00:00
4 changed files with 50 additions and 24 deletions

View File

@ -1,23 +1,24 @@
<template>
<card class="filters has-overflow">
<fancycheckbox v-model="params.filter_include_nulls">
{{ $t('filters.attributes.includeNulls') }}
</fancycheckbox>
<fancycheckbox
v-model="filters.requireAllFilters"
@change="setFilterConcat()"
>
{{ $t('filters.attributes.requireAll') }}
</fancycheckbox>
<div class="field">
<label class="label">
<fancycheckbox v-model="params.filter_include_nulls">
{{ $t('filters.attributes.includeNulls') }}
</fancycheckbox>
<fancycheckbox
v-model="filters.requireAllFilters"
@change="setFilterConcat()"
>
{{ $t('filters.attributes.requireAll') }}
</fancycheckbox>
<fancycheckbox @change="setDoneFilter" v-model="filters.done">
{{ $t('filters.attributes.showDoneTasks') }}
</label>
<div class="control">
<fancycheckbox @change="setDoneFilter" v-model="filters.done">
{{ $t('filters.attributes.showDoneTasks') }}
</fancycheckbox>
</div>
</fancycheckbox>
<fancycheckbox
v-if="!$route.name.includes('list.kanban') || !$route.name.includes('list.table')"
v-model="sortAlphabetically"
>
{{ $t('filters.attributes.sortAlphabetically') }}
</fancycheckbox>
</div>
<div class="field">
<label class="label">{{ $t('misc.search') }}</label>
@ -190,6 +191,7 @@ import NamespaceService from '@/services/namespace'
import EditLabels from '@/components/tasks/partials/editLabels.vue'
import {objectToSnakeCase} from '@/helpers/case'
import {getDefaultParams} from '@/components/tasks/mixins/taskList'
// FIXME: merge with DEFAULT_PARAMS in taskList.js
const DEFAULT_PARAMS = {
@ -220,6 +222,8 @@ const DEFAULT_FILTERS = {
namespace: '',
}
export const ALPHABETICAL_SORT = 'title'
export default {
name: 'filters',
components: {
@ -272,6 +276,18 @@ export default {
},
},
computed: {
sortAlphabetically: {
get() {
return this.params?.sort_by?.find(sortBy => sortBy === ALPHABETICAL_SORT) !== undefined
},
set(sortAlphabetically) {
this.params.sort_by = sortAlphabetically
? [ALPHABETICAL_SORT]
: getDefaultParams().sort_by
this.change()
},
},
foundLabels() {
return this.$store.getters['labels/filterLabelsByQuery'](this.labels, this.query)
},

View File

@ -44,7 +44,6 @@ export default {
params = null,
forceLoading = false,
) {
// Because this function is triggered every time on topNavigation, we're putting a condition here to only load it when we actually want to show tasks
// FIXME: This is a bit hacky -> Cleanup.
if (

View File

@ -373,6 +373,7 @@
"includeNulls": "Include Tasks which don't have a value set",
"requireAll": "Require all filters to be true for a task to show up",
"showDoneTasks": "Show Done Tasks",
"sortAlphabetically": "Sort Alphabetically",
"enablePriority": "Enable Filter By Priority",
"enablePercentDone": "Enable Filter By Percent Done",
"dueDateRange": "Due Date Range",

View File

@ -81,7 +81,7 @@
:disabled="!canWrite"
item-key="id"
:component-data="{
class: { 'dragging-disabled': !canWrite },
class: { 'dragging-disabled': !canWrite || isAlphabeticalSorting },
dpschen marked this conversation as resolved Outdated

The canDrag is misleading here because dragging could also be disabled if canWrite is false (and thus the invertion true).

Make isAlphabeticalSorting a computed and you don't have to double invert it (since
its result is inverted inside the computed).

The `canDrag` is misleading here because dragging could also be disabled if `canWrite` is false (and thus the invertion `true`). Make `isAlphabeticalSorting` a computed and you don't have to double invert it (since its result is inverted inside the computed).
}"
>
<template #item="{element: t}">
@ -148,6 +148,7 @@ import {HAS_TASKS} from '@/store/mutation-types'
import Nothing from '@/components/misc/nothing.vue'
import Pagination from '@/components/misc/pagination.vue'
import Popup from '@/components/misc/popup'
import { ALPHABETICAL_SORT } from '@/components/list/partials/filters'
import draggable from 'vuedraggable'
import {calculateItemPosition} from '../../../helpers/calculateItemPosition'
@ -206,6 +207,9 @@ export default {
saveListView(this.$route.params.listId, this.$route.name)
},
computed: {
isAlphabeticalSorting() {
return this.params.sort_by.find( sortBy => sortBy === ALPHABETICAL_SORT ) !== undefined
},
firstNewPosition() {
if (this.tasks.length === 0) {
return 0
@ -254,12 +258,18 @@ export default {
focusNewTaskInput() {
this.$refs.newTaskInput.$refs.newTaskInput.focus()
},
updateTaskList(task) {
const tasks = [
task,
...this.tasks,
]
this.tasks = tasks
updateTaskList( task ) {
if ( this.isAlphabeticalSorting ) {
// reload tasks with current filter and sorting
this.loadTasks(1, undefined, undefined, true)
}
dpschen marked this conversation as resolved Outdated

Still not quite happy with reloading everything. This gives a very noticable load flickering. I get why it is nessecary though. Maybe only reload if there are filters set and otherwise prepend the task as done already?

Still not quite happy with reloading everything. This gives a very noticable load flickering. I get why it is nessecary though. Maybe only reload if there are filters set and otherwise prepend the task as done already?

The sorting should probably happen on client and server side.
This way the client can sort already what he is aware of and fetches an update.

The sorting should probably happen on client _and_ server side. This way the client can sort already what he is aware of and fetches an update.

Since this would require a bit of work, I think we should do this in another PR and keep the current logic of prepending new tasks to the list.

Since this would require a bit of work, I think we should do this in another PR and keep the current logic of prepending new tasks to the list.

So should I do reload only if filters and/or sorts are set ?

So should I do reload only if filters and/or sorts are set ?

I think that would be a good idea.

I think that would be a good idea.
else {
dpschen marked this conversation as resolved
Review

Instead of putting this in a new function just make a comment:

// reload tasks with current filter and sorting
this.loadTasks(1, undefined, undefined, true)
Instead of putting this in a new function just make a comment: ```js // reload tasks with current filter and sorting this.loadTasks(1, undefined, undefined, true) ```
this.tasks = [
task,
konrad marked this conversation as resolved Outdated

Why did you comment this? If it is not used, remove it.

Why did you comment this? If it is not used, remove it.
...this.tasks,
dpschen marked this conversation as resolved
Review

Merge this:

this.tasks = [
	task,
    ...this.tasks,
]
Merge this: ```js this.tasks = [ task, ...this.tasks, ] ```
]
}
this.$store.commit(HAS_TASKS, true)

This should use the default params instead of undefined.

This should use the default params instead of `undefined`.

Default parameters will overwrite the current filtering,searching etc, no?

Idea here is to preserve it

Default parameters will overwrite the current filtering,searching etc, no? Idea here is to preserve it

That's actually the idea behind the default paramters. If they are set, it will first look for the params in the data variables of the component and use them. The function parameters are only meant to override that.

Passing in undefined is, well, undefined behaviour.

That's actually the idea behind the default paramters. If they are set, it will first look for the params in the data variables of the component and use them. The function parameters are only meant to override that. Passing in `undefined` is, well, undefined behaviour.

Do you mean something like this:

	reloadTasksWithCurrentFilterAndSorting(){
		this.loadTasks(1, DEFAULT_PARAMS.s, DEFAULT_PARAMS, true)
	},

Where DEFAULT PARAMS are:

// FIXME: merge with DEFAULT_PARAMS in taskList.js
export const DEFAULT_PARAMS = {
	sort_by: [],
	order_by: [],
	filter_by: [],
	filter_value: [],
	filter_comparator: [],
	filter_include_nulls: true,
	filter_concat: 'or',
	s: '',
}
Do you mean something like this: reloadTasksWithCurrentFilterAndSorting(){ this.loadTasks(1, DEFAULT_PARAMS.s, DEFAULT_PARAMS, true) }, Where DEFAULT PARAMS are: ~~~ // FIXME: merge with DEFAULT_PARAMS in taskList.js export const DEFAULT_PARAMS = { sort_by: [], order_by: [], filter_by: [], filter_value: [], filter_comparator: [], filter_include_nulls: true, filter_concat: 'or', s: '', } ~~~

No, I mean the default parameters for the variables as they are declared in the function. You should call this.loadTasks with just the page function parameter specified (the first)

No, I mean the default parameters for the variables as they are declared in the function. You should call `this.loadTasks` with just the page function parameter specified (the first)

Sure but this would break the behavior since I need forceLoading ( the 4th parameter to be set ). undefined means take the default, it was my bad for passing undefined for page but otherwise it should be this.loadTasks(1, undefined, undefined, true)

Sure but this would break the behavior since I need forceLoading ( the 4th parameter to be set ). undefined means take the default, it was my bad for passing undefined for page but otherwise it should be this.loadTasks(1, undefined, undefined, true)

Please pass in '' for the search param and null for the params param so it matches the function declaration: https://kolaente.dev/vikunja/frontend/src/branch/main/src/components/tasks/mixins/taskList.js#L41-L46

Please pass in `''` for the search param and `null` for the params param so it matches the function declaration: https://kolaente.dev/vikunja/frontend/src/branch/main/src/components/tasks/mixins/taskList.js#L41-L46
},
editTask(id) {