vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

feature/vue3-implementation-improvements #814

Closed
dpschen wants to merge 0 commits from feature/vue3-implementation-improvements into vue3
pull from: feature/vue3-implementation-improvements
merge into: vikunja:vue3
Conversation 12 Commits - Files Changed -
dpschen commented 2021-10-03 13:04:45 +00:00
Member
Copy Link
No description provided.
dpschen added a new dependency 2021-10-03 13:13:32 +00:00
#815 vue3
dpschen force-pushed feature/vue3-implementation-improvements from b8acc167be to 2ce6dd1fb3 2021-10-03 13:27:55 +00:00 Compare
dpschen requested review from konrad 2021-10-03 13:30:51 +00:00
dpschen added the
help wanted
kind/feature
 labels 2021-10-03 13:32:11 +00:00
dpschen removed a dependency 2021-10-03 15:53:22 +00:00
#815 vue3
dpschen added a new dependency 2021-10-03 15:53:40 +00:00
#815 vue3
dpschen force-pushed feature/vue3-implementation-improvements from 2ce6dd1fb3 to 8e7ce0504a 2021-10-03 15:56:32 +00:00 Compare
dpschen force-pushed feature/vue3-implementation-improvements from 8e7ce0504a to 6cc66e2ac7 2021-10-03 16:13:50 +00:00 Compare
konrad commented 2021-10-03 16:41:22 +00:00
Owner
Copy Link

Closing in favor of #815

Closing in favor of https://kolaente.dev/vikunja/frontend/pulls/815
dpschen commented 2021-10-03 17:05:12 +00:00
Author
Member
Copy Link

Closing in favor of #815

I think you confused this with #678

This branch consists of some changes that I want to merge into #815 instead.

With this branch here I'm not as confident as with the changes in #815 because there are some tests failing that didn't before (task.spec.js). But I also fixed some issues.

> Closing in favor of https://kolaente.dev/vikunja/frontend/pulls/815 I think you confused this with https://kolaente.dev/vikunja/frontend/pulls/678 This branch consists of some changes that I want to merge into https://kolaente.dev/vikunja/frontend/pulls/815 instead. With this branch here I'm not as confident as with the changes in #815 because there are some tests failing that didn't before (`task.spec.js`). But I also fixed some issues.
konrad commented 2021-10-03 18:43:12 +00:00
Owner
Copy Link

I think you confused this with #678

Whoops, you're correct.

With this branch here I'm not as confident as with the changes in #815 because there are some tests failing that didn't before (task.spec.js). But I also fixed some issues.

It looks like the test is failing because the request itself is broken - not sure why though: image

> I think you confused this with #678 Whoops, you're correct. > With this branch here I'm not as confident as with the changes in #815 because there are some tests failing that didn't before (task.spec.js). But I also fixed some issues. It looks like the test is failing because the request itself is broken - not sure why though: ![image](/attachments/da64f4a4-4551-43b8-b144-c51fb873d19d)
image.png
132 KiB
dpschen commented 2021-10-03 21:38:22 +00:00
Author
Member
Copy Link

Yes :D this is also where I currently hang. I think with some of the store updates I break some of the models. Also not sure why yet 🤷‍♂️

Yes :D this is also where I currently hang. I think with some of the store updates I break some of the models. Also not sure why yet 🤷‍♂️
dpschen force-pushed feature/vue3-implementation-improvements from 6cc66e2ac7 to 8d52b312f6 2021-10-04 12:10:15 +00:00 Compare
dpschen force-pushed feature/vue3-implementation-improvements from 5c291530ab to a38cbbb445 2021-10-07 10:24:55 +00:00 Compare
dpschen commented 2021-10-11 10:35:59 +00:00
Author
Member
Copy Link

It looks like the test is failing because the request itself is broken - not sure why though: image

I got a bit further with this issue👎

I think that when you use this branch instead of main the delete command is using the wrong listId in the request — 0 instead of 1. Why that is I'm not sure yet. Probably something going wrong when initially creating / reading in the modal.

> It looks like the test is failing because the request itself is broken - not sure why though: ![image](/attachments/da64f4a4-4551-43b8-b144-c51fb873d19d) I got a bit further with this issue👎 I think that when you use this branch instead of main the delete command is using the wrong `listId` in the request — `0` instead of `1`. Why that is I'm not sure yet. Probably something going wrong when initially creating / reading in the modal.
dpschen force-pushed feature/vue3-implementation-improvements from 8020d6d963 to 5be6be98ab 2021-10-11 16:08:47 +00:00 Compare
dpschen force-pushed feature/vue3-implementation-improvements from 5be6be98ab to 63c8de0234 2021-10-11 16:14:15 +00:00 Compare
dpschen force-pushed feature/vue3-implementation-improvements from 63c8de0234 to fd77aaa123 2021-10-11 16:18:14 +00:00 Compare
dpschen changed title from WIP: feature/vue3-implementation-improvements to feature/vue3-implementation-improvements 2021-10-11 16:24:21 +00:00
dpschen added 1 commit 2021-10-11 16:28:50 +00:00
continuous-integration/drone/pr Build is passing Details
18d7ca0820
feat: also use createRandomID helper in editor
dpschen added 4 commits 2021-10-11 16:40:22 +00:00
continuous-integration/drone/push Build is failing Details
36d4599276 fix: unassign user success messgage (#831) 
Co-authored-by: Dominik Pschenitschni <mail@celement.de>
Reviewed-on: #831
Reviewed-by: konrad <k@knt.li>
Co-authored-by: dpschen <dpschen@noreply.kolaente.de>
Co-committed-by: dpschen <dpschen@noreply.kolaente.de>
continuous-integration/drone/push Build is passing Details
3c5c3cad10 chore: remove unneeded babel packages and add peerDependencies (#828) 
Co-authored-by: Dominik Pschenitschni <mail@celement.de>
Reviewed-on: #828
Reviewed-by: konrad <k@knt.li>
Co-authored-by: dpschen <dpschen@noreply.kolaente.de>
Co-committed-by: dpschen <dpschen@noreply.kolaente.de>
continuous-integration/drone/pr Build is passing Details
continuous-integration/drone/push Build is passing Details
17d791027c
feat: add legacy build
continuous-integration/drone/pr Build is passing Details
5cfb99bfc2
Merge branch 'main' into feature/vue3-implementation-improvements 
# Conflicts:
#	package.json
#	vite.config.js
#	yarn.lock
dpschen added 3 commits 2021-10-11 17:17:31 +00:00
d1b61a1489
feat: some vue3 package improvements 
- add codemirror as direct dependency
- remove @vue/runtime-dom (was needed for volar with vue2)
- remove setting for using volar with vue2
continuous-integration/drone/pr Build is failing Details
18c31482df
fix: remove side effect from computed 
was firing a commit
dpschen added 1 commit 2021-10-11 17:25:45 +00:00
continuous-integration/drone/pr Build is failing Details
cc8b03778c
fix: use correct method for fakers uuid
dpschen added a new dependency 2021-10-11 18:09:04 +00:00
#832 feature/vue3-async-await
konrad commented 2021-10-11 18:38:05 +00:00
Owner
Copy Link

I think that when you use this branch instead of main the delete command is using the wrong listId in the request — 0 instead of 1. Why that is I'm not sure yet. Probably something going wrong when initially creating / reading in the modal.

Sounds like the listId is not set properly and it is then using the default which is 0. (0 is the default to match Go's null value)

> I think that when you use this branch instead of main the delete command is using the wrong listId in the request — 0 instead of 1. Why that is I'm not sure yet. Probably something going wrong when initially creating / reading in the modal. Sounds like the listId is not set properly and it is then using the default which is 0. (0 is the default to match Go's null value)
dpschen commented 2021-10-11 19:02:00 +00:00
Author
Member
Copy Link

Sounds like the listId is not set properly and it is then using the default which is 0. (0 is the default to match Go's null value)

Yeah I found out how to solve it. But not the issue :D
The fixes are in 0b68a473ef and d7ed5b8f11

> Sounds like the listId is not set properly and it is then using the default which is 0. (0 is the default to match Go's null value) Yeah I found out how to solve it. But not the issue :D The fixes are in https://kolaente.dev/vikunja/frontend/commit/0b68a473efdb2ca39343be64e160fe489e4fcc1c and https://kolaente.dev/vikunja/frontend/commit/d7ed5b8f1178e634b4776c567fccc772da21b319
dpschen added 2 commits 2021-10-11 19:40:51 +00:00
continuous-integration/drone/pr Build is passing Details
50fa592aad
fix: wrong async order
dpschen commented 2021-10-11 19:49:20 +00:00
Author
Member
Copy Link

@konrad Since this passes all tests now I guess this can be merged in the vue3 branch after you checked it.

@konrad Since this passes all tests now I guess this can be merged in the vue3 branch after you checked it.
👍 1
konrad requested changes 2021-10-11 21:09:41 +00:00
konrad left a comment
Owner

Looks great overall! I do have some comments though.

Looks great overall! I do have some comments though.
src/components/tasks/partials/editAssignees.vue Outdated
@ -74,6 +74,7 @@ export default {
this.assignees = value
},
immediate: true,
deept: true,
konrad commented 2021-10-11 21:01:23 +00:00
Owner

Shouldn't that be deep? (no t)

Shouldn't that be `deep`? (no `t`)
dpschen commented 2021-10-11 22:01:27 +00:00
Author
Member

Good catch! :)

Good catch! :)
dpschen marked this conversation as resolved
src/main.ts
@ -97,0 +97,4 @@
// error(err)
// } else {
console.error(err, vm, info)
// }
konrad commented 2021-10-11 21:02:35 +00:00
Owner

What should that do? The commented code doesn't feel right, please either remove it or replace it with something that handles the error.

What should that do? The commented code doesn't feel right, please either remove it or replace it with something that handles the error.
dpschen commented 2021-10-12 14:12:08 +00:00
Author
Member

This is part of something that I want to tackle in detail in #833.

This is part of something that I want to tackle in detail in https://kolaente.dev/vikunja/frontend/pulls/833.
konrad commented 2021-10-13 16:45:10 +00:00
Owner

Then maybe that should go in first?

Then maybe that should go in first?
dpschen commented 2021-10-13 16:51:36 +00:00
Author
Member

#833 is based on this pull request

#833 is based on this pull request
konrad commented 2021-10-13 16:58:27 +00:00
Owner

Ah I see, makes sense.

Ah I see, makes sense.
dpschen commented 2021-10-13 17:10:28 +00:00
Author
Member

Just to clarify: I'm 100% on your side here. This should not be included when the vue3 transition is finished.

I just let this in here since in my head I "marked" the error handling as unfinished :D

Just to clarify: I'm 100% on your side here. This should not be included when the vue3 transition is finished. I just let this in here since in my head I "marked" the error handling as unfinished :D
konrad marked this conversation as resolved
src/store/modules/kanban.js Outdated
@ -173,0 +155,4 @@
for (const b in state.buckets) {
if (state.buckets[b].id === task.bucketId) {
for (const t in state.buckets[b].tasks) {
if (state.buckets[b].tasks[t].id === task.id) {
konrad commented 2021-10-11 21:04:27 +00:00
Owner

What about using findIndexById here? At least to replace the inner loop?

What about using `findIndexById` here? At least to replace the inner loop?
👍 1
dpschen marked this conversation as resolved
src/store/modules/kanban.js Outdated
@ -259,0 +257,4 @@
// }
// }
// return condition
// })
konrad commented 2021-10-11 21:04:44 +00:00
Owner

Please remove the commented code.

Please remove the commented code.
👍 1
dpschen marked this conversation as resolved
src/store/modules/kanban.js Outdated
@ -335,0 +344,4 @@
id: updatedBucketData.id,
listId: updatedBucketData.listId || oldBucket.listId,
title: oldBucket.title, // can't be empty in request
// ...bucket,
konrad commented 2021-10-11 21:04:59 +00:00
Owner

Please remove the commented code.

Please remove the commented code.
👍 1
dpschen marked this conversation as resolved
src/views/namespaces/settings/delete.vue
@ -24,1 +18,3 @@
}
computed: {
namespace() {
return this.$store.getters['namespaces/getNamespaceById'](this.$route.params.id)
konrad commented 2021-10-11 21:07:55 +00:00
Owner

With vue 2, parameters like this.$route.params.id sometimes were a string - has that changed with vue 3? If not, we'll need to convert that to an int here.

With vue 2, parameters like `this.$route.params.id` sometimes were a string - has that changed with vue 3? If not, we'll need to convert that to an int here.
dpschen commented 2021-10-12 14:02:17 +00:00
Author
Member

I had some problems so I changed equality of how the getNamespaceById getter compares the ids. It's not strict (==) so it doesn't matter if the values type is string or number. Maybeee I also did it in case we want to change the ids from number to string.

I had some problems so I changed equality of how the `getNamespaceById` getter compares the ids. [It's not strict](https://kolaente.dev/dpschen/frontend/src/branch/feature/vue3-implementation-improvements/src/store/modules/namespaces.js#L93) (`==`) so it doesn't matter if the values type is string or number. Maybeee I also did it in case we want to change the ids from number to string.
👍 1
konrad marked this conversation as resolved
dpschen added a new dependency 2021-10-12 14:05:29 +00:00
#833 feature/vue3-global-error-management
dpschen removed a dependency 2021-10-12 14:12:51 +00:00
#832 feature/vue3-async-await
konrad approved these changes 2021-10-13 18:01:36 +00:00
konrad commented 2021-10-13 18:55:43 +00:00
Owner
Copy Link

@dpschen Could you look into the test failure?

@dpschen Could you look into the test failure?
dpschen commented 2021-10-13 19:29:12 +00:00
Author
Member
Copy Link

@dpschen Could you look into the test failure?

for sure! seems like I broke something again with the review changes :)

> @dpschen Could you look into the test failure? for sure! seems like I broke something again with the review changes :)
dpschen force-pushed feature/vue3-implementation-improvements from 920fdebf3c to fa54e1f1de 2021-10-15 17:57:28 +00:00 Compare
dpschen commented 2021-10-15 18:09:08 +00:00
Author
Member
Copy Link

@konrad I fast-forwared the vue3 branch. I didn't want to have a merge commit.
Maybe this was wrong 🤷‍♂️ ?

@konrad I fast-forwared the vue3 branch. I didn't want to have a merge commit. Maybe this was wrong 🤷‍♂️ ?
dpschen removed a dependency 2021-10-15 18:09:35 +00:00
#815 vue3
dpschen closed this pull request 2021-10-15 18:09:49 +00:00
dpschen removed a dependency 2021-10-15 18:32:01 +00:00
#833 feature/vue3-global-error-management
konrad commented 2021-10-15 18:50:32 +00:00
Owner
Copy Link

@dpschen Maybe 🤔

@dpschen Maybe 🤔
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
konrad
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#814
No description provided.
Delete Branch "feature/vue3-implementation-improvements"

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?