Make add task a component #520

Merged
konrad merged 39 commits from sytone/frontend:main into main 2021-07-17 21:21:47 +00:00
Contributor

Add add-task to home
Change add task to component for lists
Made welcome update on time.. because.

Add add-task to home Change add task to component for lists Made welcome update on time.. because.
sytone added 1 commit 2021-05-26 20:45:24 +00:00
continuous-integration/drone/pr Build is passing Details
1200f0b416
Make add task a component
Add add-task to home
Change add task to component for lists
Made welcome update on time.. because.
Author
Contributor

As a note I have never worked in Vue before so some approaches may be incorrect I did a bit of quick reading and hopfully have the right approach. I also rarely work in javascript so there is that 😄

As a note I have never worked in Vue before so some approaches may be incorrect I did a bit of quick reading and hopfully have the right approach. I also rarely work in javascript so there is that 😄
sytone added 1 commit 2021-05-26 20:59:49 +00:00
continuous-integration/drone/pr Build is passing Details
a4b64f9bc6
Merge remote-tracking branch 'upstream/main'
konrad requested changes 2021-05-26 21:22:54 +00:00
konrad left a comment
Owner

Thanks for the PR!

As a note I have never worked in Vue before so some approaches may be incorrect I did a bit of quick reading and hopfully have the right approach. I also rarely work in javascript so there is that 😄

I think that wasn't too bad. I do have some general nitpicks though:

  1. Please always use ' instead of ". That way it is the same throughout the codebase.
  2. Please remove the semicolons. Same reason, to have a consistent style throughout the codebase.

(..and sorry if I'm critizing a lot, I don't mean to be harsh or anything)

Thanks for the PR! > As a note I have never worked in Vue before so some approaches may be incorrect I did a bit of quick reading and hopfully have the right approach. I also rarely work in javascript so there is that 😄 I think that wasn't too bad. I do have some general nitpicks though: 1. Please always use `'` instead of `"`. That way it is the same throughout the codebase. 2. Please remove the semicolons. Same reason, to have a consistent style throughout the codebase. (..and sorry if I'm critizing a lot, I don't mean to be harsh or anything)
.editorconfig Outdated
@ -0,0 +5,4 @@
[*]
indent_style = tab
end_of_line = crlf

That should be lf

That should be `lf`
sytone marked this conversation as resolved
@ -0,0 +35,4 @@
import LabelModel from "../../models/label";
import { HAS_TASKS } from "@/store/mutation-types";
// import Nothing from "@/components/misc/nothing";

If the component is not used, please remove the import.

If the component is not used, please remove the import.
sytone marked this conversation as resolved
@ -0,0 +75,4 @@
}
this.showError = false;
let task = new TaskModel({

This should be const instead of let.

This should be `const` instead of `let`.
sytone marked this conversation as resolved
@ -0,0 +77,4 @@
let task = new TaskModel({
title: this.newTaskText,
listId: this.listId

Needs a , at the end.

Needs a `,` at the end.
sytone marked this conversation as resolved
@ -0,0 +81,4 @@
});
if (this.listId === undefined) {
// TODO: Have a default list in settings.

That's something that would be required to get this merged. I'd like to make that a setting at the api level though.

Another approach would be to only merge the refactoring of the component and add the rest (=adding a task from the home page) later, once the default list is implemented.

That's something that would be required to get this merged. I'd like to make that a setting at the api level though. Another approach would be to only merge the refactoring of the component and add the rest (=adding a task from the home page) later, once the default list is implemented.
Author
Contributor

The API is completed now and I update updated the code to use the settings.

The API is completed now and I update updated the code to use the settings.
sytone marked this conversation as resolved
@ -0,0 +88,4 @@
.create(task)
.then(task => {
// this.tasks.push(task);
// this.sortTasks();

If it's not used, please remove it.

If it's not used, please remove it.
sytone marked this conversation as resolved
@ -2,3 +2,3 @@
<div class="content has-text-centered">
<h2>
Hi {{ userInfo.name !== '' ? userInfo.name : userInfo.username }}!
{{ welcomePrefix }}

Would you mind moving this to a separate PR?

Would you mind moving this to a separate PR?
Author
Contributor

np, reverted that change for this PR.

np, reverted that change for this PR.
sytone marked this conversation as resolved
@ -45,0 +64,4 @@
mounted() {
const timer = window.setTimeout(
this.updateWelcome,
this.updateWelcomeInterval

I'd set that statically and to a much higher value - or maybe even remove the timeout alltogether. I don't think people will let the start page of Vikunja sit still all day without using it and then complain about the welcome text not updating. Should be enough to only render it when the component is rendered. And then this whole thing could be refactored into a computed property.

I'd set that statically and to a much higher value - or maybe even remove the timeout alltogether. I don't think people will let the start page of Vikunja sit still all day without using it and then complain about the welcome text not updating. Should be enough to only render it when the component is rendered. And then this whole thing could be refactored into a computed property.
Author
Contributor

NA, will move to seperate PR.

NA, will move to seperate PR.
sytone marked this conversation as resolved
@ -45,0 +66,4 @@
this.updateWelcome,
this.updateWelcomeInterval
);
this.$on("hook:destroyed", () => window.clearTimeout(timer));

Please use the destroyed method for this (if we're keeping the timeout).

Please use the `destroyed` method for this (if we're keeping the timeout).
Author
Contributor

NA, moved to seperate PR.

NA, moved to seperate PR.
sytone marked this conversation as resolved
@ -66,1 +93,3 @@
}
methods: {
updateTaskList() {
this.showTasksKey += 1;

What is that good for? AFAIU the only place this is used is the key on the task list component? Why does that component need a key?

What is that good for? AFAIU the only place this is used is the `key` on the task list component? Why does that component need a key?
Author
Contributor

I looked up the best way to refresh a vue component when there was a change, this was the recommended way outside of manually invalidating and causing a render which was a sledgehammer.

I looked up the best way to refresh a vue component when there was a change, this was the recommended way outside of manually invalidating and causing a render which was a sledgehammer.
sytone marked this conversation as resolved
@ -67,0 +103,4 @@
} else {
this.welcomePrefix = "Good Evening";
}
this.$options.timer = window.setTimeout(

Maybe an interval would be better suited here?

Maybe an interval would be better suited here?
Author
Contributor

NA moving to seperate PR.

NA moving to seperate PR.
sytone marked this conversation as resolved
@ -199,3 +203,4 @@
FilterPopup,
SingleTaskInList,
EditTask,
AddTask

Please add a semicolon at the end.

Please add a semicolon at the end.
sytone marked this conversation as resolved
Author
Contributor

I made a API change to add the setting, will focus on that being done and then come back to this.

I made a API change to add the setting, will focus on that being done and then come back to this.
sytone added 1 commit 2021-06-02 16:26:57 +00:00
sytone added 7 commits 2021-06-02 19:24:10 +00:00
sytone requested review from konrad 2021-06-02 19:24:29 +00:00
konrad requested changes 2021-06-02 21:34:17 +00:00
package.json Outdated
@ -63,3 +2,1 @@
"root": true,
"env": {
"node": true
"name": "vikunja-frontend",

Please don't reindent the file - that makes it harder to see the changes. And it is two spaces in the other json files as well. Should be the same style everywhere.

Please also update the .editorconfig.

Please don't reindent the file - that makes it harder to see the changes. And it is two spaces in the other json files as well. Should be the same style everywhere. Please also update the `.editorconfig`.
Author
Contributor

switched to 2 spaced, as a note the swagger is 4 spaces which is where I got it from.

switched to 2 spaced, as a note the swagger is 4 spaces which is where I got it from.
konrad marked this conversation as resolved
@ -0,0 +1,214 @@
<template>
<div class="field is-grouped" v-if="validListIdAvailable">
The validation error should be in this component as well: https://kolaente.dev/vikunja/frontend/src/branch/main/src/views/list/views/List.vue#L82-L84
Author
Contributor

Added and will verify.

Added and will verify.
sytone marked this conversation as resolved
@ -3,10 +3,16 @@
<h2>
Hi {{ userInfo.name !== '' ? userInfo.name : userInfo.username }}!
</h2>
<div>{{ defaultListId }}</div>

I suppose that's just left from debugging?

Maybe check out the vue devtools extension, it is great to look into the values of props and data in a component.

I suppose that's just left from debugging? Maybe check out the vue devtools extension, it is great to look into the values of props and data in a component.
Author
Contributor

yea, was from merge and should not be there. I am using the tools now.

yea, was from merge and should not be there. I am using the tools now.
konrad marked this conversation as resolved
@ -66,1 +82,3 @@
}
methods: {
updateTaskList() {
this.showTasksKey += 1;

I'd rather refactor the tasks component to accept an array of tasks as a prop to avoid retriggering the component. Or putting the tasks in vuex.

Updating the key to retrigger a render of the component does not feel like a great solution. (I know I'm guilty of using hacks like these myself, but better do it right the first time 🙃 )

I'd rather refactor the tasks component to accept an array of tasks as a prop to avoid retriggering the component. Or putting the tasks in vuex. Updating the key to retrigger a render of the component does not feel like a great solution. (I know I'm guilty of using hacks like these myself, but better do it right the first time 🙃 )
Author
Contributor

This would move the task query logic to Home and I am not sure if that makes sense, adding a new prop to the Show Tasks does not look overly complex.

Using vuex woultake some time as I have not used it nor do I know how the stores would be synced.

This would move the task query logic to Home and I am not sure if that makes sense, adding a new prop to the Show Tasks does not look overly complex. Using vuex woultake some time as I have not used it nor do I know how the stores would be synced.

I see - my main issue with this is it will requery all data from the server even though all tasks (the old ones which were previously shown and the newly created) are already availabl on the client. The flow would be something like this: The new add-tasks component would emit a @task-created event with the new task model. The outer component then listens to that event and adds the tasks to the array of existing tasks.

This also applies to the list view.

Given how complex this is I think we could move this PR without the "clean" solution and add that in another PR.

I see - my main issue with this is it will requery all data from the server even though all tasks (the old ones which were previously shown and the newly created) are already availabl on the client. The flow would be something like this: The new `add-tasks` component would emit a `@task-created` event with the new task model. The outer component then listens to that event and adds the tasks to the array of existing tasks. This also applies to the list view. Given how complex this is I think we could move this PR without the "clean" solution and add that in another PR.
@ -88,2 +76,4 @@
"
>
This list is currently empty.
<a @click="$refs.newTaskInput.focus()">Create a new task.</a>

Does that still work when the add task input is in a new component?

Does that still work when the add task input is in a new component?
Author
Contributor

yes it does, I validated in a few different combinations and looked this up on how to do it with Vue

yes it does, I validated in a few different combinations and looked this up on how to do it with Vue
sytone marked this conversation as resolved
Owner

Oh one more thing: Would you mind remonving the ;? I like the style without them much more and the rest of the codebase doesn't really have them - it's a question of consistency at the end of the day.

Oh one more thing: Would you mind remonving the `;`? I like the style without them much more and the rest of the codebase doesn't really have them - it's a question of consistency at the end of the day.
Author
Contributor

semi colons are removed and added rule to catch it.

semi colons are removed and added rule to catch it.
sytone added 6 commits 2021-06-07 16:08:07 +00:00
sytone added 1 commit 2021-06-07 22:12:53 +00:00
continuous-integration/drone/pr Build is passing Details
a9728f774d
Add settings lookup to add-task
Enable specification of List ID for list-search
Add default list to user settings
Add default setting
Update settings UI for default list
sytone added 2 commits 2021-06-08 15:25:43 +00:00
sytone requested review from konrad 2021-06-08 15:26:10 +00:00
Author
Contributor

The only open point is the refresh of the list when a task is added, moving the ask logic to the Home component seems ikky as now home cares about tasks where it should just hold tasks. That may just be me.

I looked a vuex but not sure where to start.

The only open point is the refresh of the list when a task is added, moving the ask logic to the Home component seems ikky as now home cares about tasks where it should just hold tasks. That may just be me. I looked a vuex but not sure where to start.
konrad requested changes 2021-06-09 13:25:38 +00:00
@ -0,0 +27,4 @@
Add
</x-button>
</p>
<p class="help is-warning" v-if="!validListIdAvailable">

This will never be shown because the whole component only appears if validListIdAvailable is true. And if that's the case, this v-if can never evaluate to true.

This will never be shown because the whole component only appears if `validListIdAvailable` is true. And if that's the case, this `v-if` can never evaluate to true.
sytone marked this conversation as resolved
@ -0,0 +29,4 @@
</p>
<p class="help is-warning" v-if="!validListIdAvailable">
No default list set. Please go to settings and specify default list.
</p>
Validation error for an empty title still missing: https://kolaente.dev/vikunja/frontend/src/branch/main/src/views/list/views/List.vue#L82-L84
Author
Contributor

Not sure what you mean by title. I have updated the component to have a div control the input vs warning message. Look in current commit.

Not sure what you mean by title. I have updated the component to have a div control the input vs warning message. Look in current commit.

The user has to enter a title for a new task. If none is provided they should see an error message as they currently do. Please correct me if I'm wrong, but I did not find that here in this new component.

The user has to enter a title for a new task. If none is provided they should see an error message as they currently do. Please correct me if I'm wrong, but I did not find that here in this new component.
sytone marked this conversation as resolved
@ -0,0 +38,4 @@
import LabelModel from '../../models/label'
import { HAS_TASKS } from '@/store/mutation-types'
// import Nothing from "@/components/misc/nothing";

Please remove unused imports.

Please remove unused imports.
Author
Contributor

Done

Done
sytone marked this conversation as resolved
@ -0,0 +59,4 @@
validListIdAvailable: false,
}
},
components: {},

Please remove the empty block.

Please remove the empty block.
sytone marked this conversation as resolved
@ -0,0 +72,4 @@
this.labelService = new LabelService()
this.labelTaskService = new LabelTaskService()
},
beforeMount() {

Could you move these variables declared inside this block into computed properties?

Could you move these variables declared inside this block into computed properties?
Author
Contributor

I was following the convention where the services were set in the created function. Can you point me to an example of how to do it in another component so I Can understand more.

I was following the convention where the services were set in the created function. Can you point me to an example of how to do it in another component so I Can understand more.

Oh I didn't mean the services, I mean the listIdForNewTask and validListIdAvailable variables declared in beforeMount().

Check out the Vue docs about computed properties and an example of using a computed property.

You'd define one computed property for each variable and then place the code inside them, something like this:

listIdForNewTask() {
	if (this.listId !== undefined) {
    	return this.listId
    }
    
    if(this.$store.state.auth.settings.defaultListId !== undefined) {
		return this.$store.state.auth.settings.defaultListId
    }
    
    return null // or undefined	
},
validListIdAvailable() {
	return this.listIdForNewTask !== undefined
}
Oh I didn't mean the services, I mean the `listIdForNewTask` and `validListIdAvailable` variables declared in `beforeMount()`. Check out the [Vue docs about computed properties](https://vuejs.org/v2/guide/computed.html#Computed-Properties) and [an example of using a computed property](https://kolaente.dev/vikunja/frontend/src/branch/main/src/components/quick-actions/quick-actions.vue#L100-L106). You'd define one computed property for each variable and then place the code inside them, something like this: ```js listIdForNewTask() { if (this.listId !== undefined) { return this.listId } if(this.$store.state.auth.settings.defaultListId !== undefined) { return this.$store.state.auth.settings.defaultListId } return null // or undefined }, validListIdAvailable() { return this.listIdForNewTask !== undefined } ```
sytone marked this conversation as resolved
@ -36,2 +36,4 @@
Multiselect,
},
props: {
listId: {

I think this should either expose the internal list or (better) use the v-model prop to populate the selected list. That one is bidirectional.

I think this should either expose the internal list or (better) [use the v-model](https://www.digitalocean.com/community/tutorials/how-to-add-v-model-support-to-custom-vue-js-components) prop to populate the selected list. That one is bidirectional.
konrad marked this conversation as resolved
@ -38,2 +43,3 @@
},
beforeMount() {
this.listSerivce = new ListService()
this.listService = new ListService()

Good catch!

Good catch!
sytone marked this conversation as resolved
@ -9,6 +9,7 @@ export default class UserSettingsModel extends AbstractModel {
discoverableByName: false,
discoverableByEmail: false,
overdueTasksRemindersEnabled: true,
defaultListId: 1,

That should be 0 - you don't know if the current user has access to that list.

That should be `0` - you don't know if the current user has access to that list.
sytone marked this conversation as resolved
@ -3,3 +3,4 @@
import UserModel from '../../models/user'
const defaultSettings = settings => {
// if (typeof settings.defaultListId === 'undefined' || settings.defaultListId === '') {

Please remove the commented code.

Please remove the commented code.
sytone marked this conversation as resolved
@ -41,3 +49,4 @@
loading: false,
currentDate: new Date(),
tasks: [],
defaultListId: undefined,

That could be a computed property, mapped from state (see below in this component)

That could be a computed property, mapped from state (see below in this component)
sytone marked this conversation as resolved
sytone added 2 commits 2021-06-15 23:46:54 +00:00
sytone added 1 commit 2021-06-16 00:00:20 +00:00
continuous-integration/drone/pr Build is failing Details
f39a4d76da
Fix nesting to show warning if list not set.
Cleanup comments
Cleanup user settings
Move default list id to computed
sytone added 1 commit 2021-06-17 19:55:37 +00:00
continuous-integration/drone/pr Build is failing Details
3a6d81d221
fixes for comments
sytone added 26 commits 2021-06-29 16:06:15 +00:00
continuous-integration/drone/push Build is passing Details
e9fdf15f23
Update dependency marked to v2.1.0 (#552)
Reviewed-on: #552
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
continuous-integration/drone/push Build is passing Details
08708927e6
Update dependency marked to v2.1.1 (#553)
Reviewed-on: #553
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
continuous-integration/drone/push Build is passing Details
fe5ecb4865
Update dependency bulma to v0.9.3 (#554)
Reviewed-on: #554
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
continuous-integration/drone/push Build is passing Details
9abe2a4de2
Update dependency eslint to v7.29.0 (#555)
Reviewed-on: #555
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
continuous-integration/drone/push Build is passing Details
81f9d62ce2
Update dependency cypress-file-upload to v5.0.8 (#556)
Reviewed-on: #556
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
continuous-integration/drone/push Build is passing Details
7f2fba8bb7
Update dependency vue-router to v3.5.2 (#557)
Reviewed-on: #557
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
continuous-integration/drone/push Build is passing Details
92d3c8521f
Update dependency jest to v27.0.5 (#558)
Reviewed-on: #558
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
continuous-integration/drone/push Build is passing Details
ab4edc17de
Update dependency marked to v2.1.2 (#559)
Reviewed-on: #559
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
continuous-integration/drone/push Build is passing Details
cdc805c8da
Refactor success and error messages
continuous-integration/drone/push Build is passing Details
f58a48bc1f
Move general settings to the top
continuous-integration/drone/push Build is passing Details
543df91aba
Fix header layout for long list titles
continuous-integration/drone/push Build is passing Details
5badb65037
Update dependency cypress to v7.6.0 (#561)
Reviewed-on: #561
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
continuous-integration/drone/push Build is passing Details
f0498fd767
Add translations (#562)
Reviewed-on: #562
Co-authored-by: konrad <konrad@kola-entertainments.de>
Co-committed-by: konrad <konrad@kola-entertainments.de>
continuous-integration/drone/push Build is passing Details
13f02c6165
Pin dependency vue-i18n to v8.24.4 (#563)
Reviewed-on: #563
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
continuous-integration/drone/push Build is passing Details
1f42d18d47
Update dependency vue-i18n to v8.24.5 (#564)
Reviewed-on: #564
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
continuous-integration/drone/push Build is passing Details
7b99d96df0
Move weblate ping to shell script
continuous-integration/drone/push Build was killed Details
7d9328735c
Fix some translation strings
continuous-integration/drone/push Build is failing Details
807685028c
Break long list titles in list overview
continuous-integration/drone/push Build is failing Details
3b23378a26
Update dependency eslint-plugin-vue to v7.12.1 (#565)
Reviewed-on: #565
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
continuous-integration/drone/push Build is passing Details
c5e53175cf
Allow failure of the weblate update step
continuous-integration/drone/push Build is passing Details
34ab4fbf5e
Update dependency marked to v2.1.3 (#567)
Reviewed-on: #567
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
continuous-integration/drone/push Build is passing Details
101a60799a
Update dependency wait-on to v6 (#568)
Reviewed-on: #568
Co-authored-by: renovate <renovatebot@kolaente.de>
Co-committed-by: renovate <renovatebot@kolaente.de>
sytone added 1 commit 2021-06-29 16:07:03 +00:00
konrad added 2 commits 2021-07-17 19:05:07 +00:00
2c4829bc04
Merge branch 'main' into pr-520-add-task-component
# Conflicts:
#	src/views/Home.vue
#	src/views/list/views/List.vue
continuous-integration/drone/pr Build is failing Details
d3c04ef738
Fix lint
konrad added 1 commit 2021-07-17 19:19:14 +00:00
continuous-integration/drone/pr Build is failing Details
7b8fbe1835
Move add task logic from list with quick add
konrad added 1 commit 2021-07-17 19:22:50 +00:00
continuous-integration/drone/pr Build was killed Details
0e02b38c1e
Actually use the list when creating a new task
konrad added 1 commit 2021-07-17 19:34:18 +00:00
continuous-integration/drone/pr Build is failing Details
e405ebe88f
Check if we have a list specified
konrad added 1 commit 2021-07-17 19:37:45 +00:00
continuous-integration/drone/pr Build was killed Details
3d18c69429
Cleanup
konrad added 1 commit 2021-07-17 19:45:41 +00:00
continuous-integration/drone/pr Build is failing Details
3d375ddf56
Fix input styles
konrad added 2 commits 2021-07-17 20:02:52 +00:00
konrad added 1 commit 2021-07-17 20:14:33 +00:00
continuous-integration/drone/pr Build is failing Details
a0f1913645
Add a v-model prop to listSearch
konrad added 1 commit 2021-07-17 20:19:54 +00:00
continuous-integration/drone/pr Build is failing Details
c1d8626fed
Add explanatory fixme
konrad approved these changes 2021-07-17 20:23:02 +00:00
konrad left a comment
Owner

I've merged this with the main branch and fixed the remaining issues. Merging when CI passes.

I've merged this with the `main` branch and fixed the remaining issues. Merging when CI passes.
konrad added 1 commit 2021-07-17 20:37:30 +00:00
continuous-integration/drone/pr Build was killed Details
1f946aa47d
Increase cypress command timeout
konrad added 1 commit 2021-07-17 20:38:14 +00:00
continuous-integration/drone/pr Build is failing Details
901aabe976
Increase cypress command timeout
konrad added 1 commit 2021-07-17 21:12:05 +00:00
continuous-integration/drone/pr Build is passing Details
dc0a2defda
Fix saving settings when no default list was selected
konrad merged commit 306a926c66 into main 2021-07-17 21:21:47 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.