Make add task a component #520
No reviewers
Labels
No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
No Milestone
No project
No Assignees
2 Participants
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#520
Loading…
Reference in New Issue
No description provided.
Delete Branch "sytone/frontend:main"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Add add-task to home
Change add task to component for lists
Made welcome update on time.. because.
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 😄
Thanks for the PR!
I think that wasn't too bad. I do have some general nitpicks though:
'
instead of"
. That way it is the same throughout the codebase.(..and sorry if I'm critizing a lot, I don't mean to be harsh or anything)
@ -0,0 +5,4 @@
[*]
indent_style = tab
end_of_line = crlf
That should be
lf
@ -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.
@ -0,0 +75,4 @@
}
this.showError = false;
let task = new TaskModel({
This should be
const
instead oflet
.@ -0,0 +77,4 @@
let task = new TaskModel({
title: this.newTaskText,
listId: this.listId
Needs a
,
at the end.@ -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.
The API is completed now and I update updated the code to use the settings.
@ -0,0 +88,4 @@
.create(task)
.then(task => {
// this.tasks.push(task);
// this.sortTasks();
If it's not used, please remove it.
@ -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?
np, reverted that change for this PR.
@ -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.
NA, will move to seperate PR.
@ -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).NA, moved to seperate PR.
@ -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?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.
@ -67,0 +103,4 @@
} else {
this.welcomePrefix = "Good Evening";
}
this.$options.timer = window.setTimeout(
Maybe an interval would be better suited here?
NA moving to seperate PR.
@ -199,3 +203,4 @@
FilterPopup,
SingleTaskInList,
EditTask,
AddTask
Please add a semicolon at the end.
I made a API change to add the setting, will focus on that being done and then come back to this.
@ -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
.switched to 2 spaced, as a note the swagger is 4 spaces which is where I got it from.
@ -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
Added and will verify.
@ -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.
yea, was from merge and should not be there. I am using the tools now.
@ -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 🙃 )
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.
@ -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?
yes it does, I validated in a few different combinations and looked this up on how to do it with Vue
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.semi colons are removed and added rule to catch it.
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.
@ -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, thisv-if
can never evaluate to true.@ -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
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.
@ -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.
Done
@ -0,0 +59,4 @@
validListIdAvailable: false,
}
},
components: {},
Please remove the empty block.
@ -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?
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
andvalidListIdAvailable
variables declared inbeforeMount()
.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:
@ -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.
@ -38,2 +43,3 @@
},
beforeMount() {
this.listSerivce = new ListService()
this.listService = new ListService()
Good catch!
@ -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.@ -3,3 +3,4 @@
import UserModel from '../../models/user'
const defaultSettings = settings => {
// if (typeof settings.defaultListId === 'undefined' || settings.defaultListId === '') {
Please remove the commented code.
@ -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)
I've merged this with the
main
branch and fixed the remaining issues. Merging when CI passes.