feat/alphabetical-sort #1162

Michaelpalacce · 2021-12-09T07:03:46Z

Michaelpalacce commented

2021-12-09 07:03:46 +00:00

Alphabetical sorting.
Disables re-ordering when applied.
Does not work with the search button as expected, but neither do the filters tbh... Works fine with the search in the filters menu.

I know we talked about having a dropdown but since this is pretty much finished I thought I'd submit a PR. I am a bit short on time these days but may submit a new PR to add the dropdown ( should be simple enough )

Alphabetical sorting. Disables re-ordering when applied. Does not work with the search button as expected, but neither do the filters tbh... Works fine with the search in the filters menu. I know we talked about having a dropdown but since this is pretty much finished I thought I'd submit a PR. I am a bit short on time these days but may submit a new PR to add the dropdown ( should be simple enough )

❤️ 1

Michaelpalacce added 3 commits 2021-12-09 07:03:47 +00:00

556f7ab6bf [alphabetical-sort] Done with checkbox

95424e4c23 [alphabetical-sort] Removed dropdwon for now

continuous-integration/drone/pr Build is failing Details

6b5d72da72 [alphabetical-sort] Added one todo

tiritibambix commented

2021-12-09 10:48:45 +00:00

First-time contributor

Yes! This is happening :D

Michaelpalacce commented

2021-12-09 11:54:04 +00:00

Fix linting issues with build

konrad requested changes 2021-12-11 12:47:52 +00:00

konrad left a comment

Hi! Thanks for the PR.

A few nits, but looks good overall.

Please also fix the lint errors, you should be able to run yarn lint --fix to fix most of them.

Hi! Thanks for the PR. A few nits, but looks good overall. Please also fix the lint errors, you should be able to run `yarn lint --fix` to fix most of them.

src/components/list/partials/filters.vue Outdated

						
				@ -493,0 +503,4 @@

								this.params.sort_by	= getDefaultParams().sort_by

								this.sortAlphabetically	= false

							} else {

								this.params.sort_by	= ['title']

konrad commented

2021-12-11 12:45:05 +00:00

Please use a const for that.

konrad marked this conversation as resolved

src/i18n/lang/cs-CZ.json Outdated

						
				@ -373,6 +373,7 @@

				      "includeNulls": "Zahrnout úkoly, které nemají nastavenou hodnotu",

				      "requireAll": "Vyžaduje aby všechny filtry odpovídaly, aby se úkol zobrazil",

				      "showDoneTasks": "Zobrazit dokončené úkoly",

				      "sortAlphabetically": "Sort Alphabetically",

konrad commented

2021-12-11 12:43:14 +00:00

This will be synced from crowdin automatically - please don't include it in this PR, only the string in en.json.

This will be synced from crowdin automatically - please don't include it in this PR, only the string in `en.json`.

konrad marked this conversation as resolved

src/views/list/views/List.vue Outdated

						
				@ -263,0 +265,4 @@

							// 	task,

							// 	...this.tasks,

							// ]

							// this.tasks = tasks

konrad commented

2021-12-11 12:45:32 +00:00

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

konrad marked this conversation as resolved

src/views/list/views/List.vue Outdated

						
				@ -264,0 +270,4 @@

							this.reloadTasksWithCurrentFilterAndSorting()

						},

						reloadTasksWithCurrentFilterAndSorting(){

							this.loadTasks(undefined, undefined, undefined, true)

konrad commented

2021-12-11 12:46:47 +00:00

This should use the default params instead of undefined.

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

Michaelpalacce commented

2021-12-15 07:00:22 +00:00

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

konrad commented

2021-12-15 20:27:23 +00:00

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.

Michaelpalacce commented

2021-12-16 21:21:46 +00:00

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: '', } ~~~

konrad commented

2021-12-16 21:24:32 +00:00

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)

Michaelpalacce commented

2021-12-16 21:36:26 +00:00

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)

konrad commented

2021-12-17 06:56:12 +00:00

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

dpschen commented

2021-12-11 12:59:29 +00:00

@Michaelpalacce @konrad

Thanks for this pull request.

This will collide with #816 if we merge it.

@Michaelpalacce @konrad Thanks for this pull request. This will collide with https://kolaente.dev/vikunja/frontend/pulls/816 if we merge it.

konrad commented

2021-12-11 13:05:09 +00:00

@dpschen Would this require a rewrite once #816 is merged or "only" a few conflicts?

dpschen commented

2021-12-11 13:08:52 +00:00

I think currently only the import

import {getDefaultParams} from "../../tasks/mixins/taskList"

EDIT: to be clear: the filepath of that import. So nothing major.

and the changed changed stuff in src/views/list/views/List.vue is conflicting.
The latter has changed a lot in my branch.

I think currently only the import ```js import {getDefaultParams} from "../../tasks/mixins/taskList" ``` **EDIT:** to be clear: the filepath of that import. So nothing major. and the changed changed stuff in `src/views/list/views/List.vue` is conflicting. The latter has changed a lot in my branch.

Michaelpalacce added 1 commit 2021-12-15 07:02:38 +00:00

continuous-integration/drone/pr Build is failing Details

1b6d50860c [alphabetical-sort] Review fixes

frederick commented

2021-12-15 07:07:11 +00:00

Hi Michaelpalacce!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1162-featalphabetical-sort--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 Michaelpalacce! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1162-featalphabetical-sort--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 reviewed 2021-12-15 20:29:00 +00:00

src/views/list/views/List.vue Outdated

						
				@ -262,2 +263,3 @@

							this.tasks = tasks

						updateTaskList() {

							this.$store.commit(HAS_TASKS, true)

							this.reloadTasksWithCurrentFilterAndSorting()

konrad commented

2021-12-15 20:29:00 +00:00

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?

dpschen commented

2021-12-15 20:43:02 +00:00

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.

konrad commented

2021-12-15 20:54:02 +00:00

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.

Michaelpalacce commented

2021-12-16 21:06:05 +00:00

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

👍 1

konrad commented

2021-12-16 21:22:31 +00:00

I think that would be a good idea.

👍 1

dpschen marked this conversation as resolved

konrad commented

2021-12-15 20:29:31 +00:00

It looks like this breaks the test. Please take a look.

dpschen requested changes 2021-12-15 20:43:10 +00:00

src/components/list/partials/filters.vue Outdated

						
				@ -190,6 +195,7 @@ import NamespaceService from '@/services/namespace'

				import EditLabels from '@/components/tasks/partials/editLabels.vue'

				import {objectToSnakeCase} from '@/helpers/case'

				import {getDefaultParams} from '../../tasks/mixins/taskList'

dpschen commented

2021-12-15 20:36:53 +00:00

Use @

Use `@`

dpschen marked this conversation as resolved

src/views/list/views/List.vue Outdated

						
				@ -225,2 +228,4 @@

					},

					methods: {

						isAlphabeticalSorting() {

							return this.sorting.find( sortBy => sortBy === 'title' ) !== undefined

dpschen commented

2021-12-15 20:40:07 +00:00

Import declared const ALPHABETICAL_SORT

Import declared const `ALPHABETICAL_SORT`

dpschen marked this conversation as resolved

Michaelpalacce commented

2021-12-16 21:39:05 +00:00

It looks like this breaks the test. Please take a look.

I don't really know much of cypress, to fix it. Seems like something basic but I cannot tell exactly what the issue is

> It looks like this breaks the test. Please take a look. I don't really know much of cypress, to fix it. Seems like something basic but I cannot tell exactly what the issue is

dpschen commented

2021-12-17 15:46:54 +00:00

@Michaelpalacce:

Something went wrong in 5808a8980b002f898ee4504f37a8491419268424.
Seems like all files were formatted.

@Michaelpalacce: Something went wrong in 5808a8980b002f898ee4504f37a8491419268424. Seems like all files were formatted.

Michaelpalacce commented

2021-12-17 15:51:44 +00:00

@Michaelpalacce:

Something went wrong in 5808a8980b002f898ee4504f37a8491419268424.
Seems like all files were formatted.

Oh wow.. I'll ammend this as soon as possible

> @Michaelpalacce: > > Something went wrong in 5808a8980b002f898ee4504f37a8491419268424. > Seems like all files were formatted. Oh wow.. I'll ammend this as soon as possible

👍 1

dpschen commented

2021-12-17 15:56:39 +00:00

If you did this, mark me.

I can check if I can find out anything about the failing test.
But might be tricky: some tests are flaky right now :/
I'm not super into Cypres aswell.

If you did this, mark me. I can check if I can find out anything about the failing test. But might be tricky: some tests are flaky right now :/ I'm not super into Cypres aswell.

Michaelpalacce force-pushed feat/alphabetical-sort from 5808a8980b to 1b6d50860c

2021-12-19 13:08:41 +00:00

Compare

Michaelpalacce added 1 commit 2021-12-19 13:09:19 +00:00

continuous-integration/drone/pr Build is failing Details

0de59a0e94 [alphabetical-sort] Ammended commit

dpschen requested changes 2021-12-19 20:02:33 +00:00

src/components/list/partials/filters.vue Outdated

						
				@ -491,2 +500,4 @@

							this.change()

						},

						// @TODO - Improve logic to work for all sorts ideally.

						setSortAlphabetically() {

dpschen commented

2021-12-19 19:59:06 +00:00

Use computed setter and getter and remove @change:

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
	},
},

Use computed setter and getter and remove `@change`: ```js 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 }, },

dpschen marked this conversation as resolved

src/views/list/views/List.vue Outdated

						
				@ -82,3 +82,3 @@

										item-key="id"

										:component-data="{

											class: { 'dragging-disabled': !canWrite },

											class: { 'dragging-disabled': !canWrite || !canDrag },

dpschen commented

2021-12-19 18:05:21 +00:00

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).

dpschen marked this conversation as resolved

src/views/list/views/List.vue Outdated

						
				@ -225,2 +229,4 @@

					},

					methods: {

						isAlphabeticalSorting() {

							return this.sorting.find( sortBy => sortBy === ALPHABETICAL_SORT ) !== undefined

dpschen commented

2021-12-19 18:09:10 +00:00

Please use params.sort_by => This way we do not need to introduce a new this.sorting variable in the mixin.

Please use `params.sort_by` => This way we do not need to introduce a new `this.sorting` variable in the mixin.

dpschen marked this conversation as resolved

src/views/list/views/List.vue

						
				@ -262,1 +264,3 @@

							this.tasks = tasks

						updateTaskList( task ) {

							if ( this.isAlphabeticalSorting() ) {

								this.reloadTasksWithCurrentFilterAndSorting()

dpschen commented

2021-12-19 18:10:51 +00:00

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) ```

dpschen marked this conversation as resolved

src/views/list/views/List.vue

						
				@ -263,0 +266,4 @@

								this.reloadTasksWithCurrentFilterAndSorting()

							}

							else {

								const tasks = [

dpschen commented

2021-12-19 18:11:37 +00:00

Merge this:

this.tasks = [
	task,
    ...this.tasks,
]

Merge this: ```js this.tasks = [ task, ...this.tasks, ] ```

dpschen marked this conversation as resolved

Michaelpalacce added 1 commit 2021-12-20 09:54:43 +00:00

continuous-integration/drone/pr Build is passing Details

cebc060714 [alphabetical-sort] Review fixes

dpschen commented

2021-12-21 12:55:57 +00:00

Okay I changed some small stuff in my branch.
I don't have the right to push to your branch @Michaelpalacce.

My Changes

I fixed a property access error that happend in the kanban view.
The sorting alphabetical doesn't work for the kanban and the table view but I think an implementation for kanban is not in the scope of this pull request. In the table soriting by title is already possible by clicking on the column header.
Making the new option work aswell might be not too complex to add here but I would prefere to improve the general loading implementation instead of wasting time by making the current implementation more complex for a single case / filter.
improved the grouping in the <filters> component. It was grouped in the field under "Show done tasks" which doesn't really make sense. I also removed the divs with the .control class since they didn't change the layout and didn't add any semantics.

Since I see this branch as a quick win to make alphabetical sorting work I think we shouldn't waste time for complex special cases. Because of that I'm fine with merging after @Michaelpalacce cherry picked my added commits.

@konrad: Anything to add?

Okay I changed some small stuff in [my branch](https://kolaente.dev/dpschen/frontend/src/branch/feat/alphabetical-sort). I don't have the right to push to your branch @Michaelpalacce. ### My Changes - I fixed a property access error that happend in the kanban view. - The sorting alphabetical doesn't work for the kanban and the table view but I think an implementation for kanban is not in the scope of this pull request. In the table soriting by title is already possible by clicking on the column header. Making the new option work aswell might be not too complex to add here but I would prefere to improve the general loading implementation instead of wasting time by making the current implementation more complex for a single case / filter. - improved the grouping in the `<filters>` component. It was grouped in the field under "Show done tasks" which doesn't really make sense. I also removed the divs with the `.control` class since they didn't change the layout and didn't add any semantics. Since I see this branch as a quick win to make alphabetical sorting work I think we shouldn't waste time for complex special cases. Because of that I'm fine with merging after @Michaelpalacce cherry picked my added commits. @konrad: Anything to add?

konrad was assigned by dpschen

2021-12-21 12:57:37 +00:00

Michaelpalacce commented

2021-12-21 12:59:18 +00:00

@dpschen yes it was decided that this will be done only for list view for now.

Also I also don't have access to your branch ( can't even see it. )
If you want I can give you access to mine so you can push any changes you want?

@dpschen yes it was decided that this will be done only for list view for now. Also I also don't have access to your branch ( can't even see it. ) If you want I can give you access to mine so you can push any changes you want?

👍 1

dpschen added 3 commits 2021-12-21 14:10:32 +00:00

895474e3bf

fix: remove error for undefined sort_by in kanban view

19bde2a86e

feat(filters): improve grouping

continuous-integration/drone/pr Build is passing Details

4d4fb89c0c

chore: remove unnecessary control wrapper

dpschen commented

2021-12-21 14:10:51 +00:00