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

feat: mark undone if task moved from isDoneBucket #3291

Merged
konrad merged 1 commits from WofWca/frontend:mark-undone-after-moving-from-done-bucket into main 2023-03-28 10:21:23 +00:00
Conversation 9 Commits 1 Files Changed 1 +8
WofWca commented 2023-03-23 15:52:06 +00:00
Contributor
Copy Link

Addresses #545 (not completely)

It already is marked as done if it's moved into the done bucket on
the backend:
6aadaaaffc/pkg/models/tasks.go (L867-L869)

Addresses #545 (not completely) It already is marked as done if it's moved _into_ the done bucket on the backend: https://kolaente.dev/vikunja/api/src/commit/6aadaaaffc1fff4a94e35e8fa3f6eab397cbc3ce/pkg/models/tasks.go#L867-L869
frederick commented 2023-03-23 15:57:21 +00:00
Member
Copy Link

Hi WofWca!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3291-mark-undone-after-moving-from-do--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 WofWca! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://3291-mark-undone-after-moving-from-do--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.
dpschen reviewed 2023-03-23 16:58:23 +00:00
src/views/project/ProjectKanban.vue Outdated
@ -426,2 +427,4 @@
taskAfter !== null ? taskAfter.kanbanPosition : null,
)
if (
oldBucket && // This shouldn't be `undefined`, but let's play it safe.
dpschen commented 2023-03-23 16:58:23 +00:00
Member

If I'm not wrong the bucketId could be 0. If that's the case we really should check for undefined instead.

If I'm not wrong the `bucketId` could be `0`. If that's the case we really should check for `undefined` instead.
WofWca commented 2023-03-23 17:52:09 +00:00
Author
Contributor

Unless we can specify the reason for it being 0 I'm for keeping the comment. Makes it clear that we don't actually know it.

Unless we can specify the reason for it being `0` I'm for keeping the comment. Makes it clear that we don't actually know it.
dpschen commented 2023-03-23 18:19:08 +00:00
Member

Its the id of the default bucket. The comment is fine

Its [the id of the default bucket](https://kolaente.dev/vikunja/frontend/src/commit/02971f6ff9c4dd98014874dbb544f4fad66eb0ab/src/models/bucket.ts#L10). The comment is fine
WofWca commented 2023-03-23 18:22:08 +00:00
Author
Contributor

By "default bucket" you mean a newly created Bucket instance, right? I don't think it's currently possible to move tasks from a bucket that hasn't been sent to the backend yet, is it?

By "default bucket" you mean a newly created `Bucket` instance, right? I don't think it's currently possible to move tasks from a bucket that hasn't been sent to the backend yet, is it?
konrad commented 2023-03-23 20:31:58 +00:00
Owner

The default bucket is the leftmost bucket, where tasks are created (or moved to) when no bucket is specified.

The default bucket is the leftmost bucket, where tasks are created (or moved to) when no bucket is specified.
WofWca commented 2023-03-24 07:06:23 +00:00
Author
Contributor

The default bucket is the leftmost bucket

IMU code is run when you drag a task between buckets or within a bucket, so the said task should always have bucketId set.

> The default bucket is the leftmost bucket IMU code is run when you drag a task between buckets or within a bucket, so the said task should always have `bucketId` set.
konrad commented 2023-03-24 11:04:49 +00:00
Owner

Yeah in the frontend there should always be a bucket id set. What is was describing is the api behaviour.

Yeah in the frontend there should always be a bucket id set. What is was describing is the api behaviour.
dpschen commented 2023-03-27 21:13:20 +00:00
Member

By 'set' you mean 'some other value than 0?

My original intend here mostly was: I see it as bad style ff a possible value of a variable is 0 (which is true here, even if it might get replaced with something different shortly after). Because of that I thought:

oldBucket !== undefined && // This shouldn't be `undefined`, but let's play it safe.

Might make sense here.

By 'set' you mean 'some other value than `0`? My original intend here mostly was: I see it as bad style ff a possible value of a variable is `0` (which is true here, even if it might get replaced with something different shortly after). Because of that I thought: ```ts oldBucket !== undefined && // This shouldn't be `undefined`, but let's play it safe. ``` Might make sense here.
konrad commented 2023-03-28 09:56:00 +00:00
Owner

By 'set' you mean 'some other value than 0?

yes, exactly.

> By 'set' you mean 'some other value than 0? yes, exactly.
WofWca commented 2023-03-28 10:02:10 +00:00
Author
Contributor

By 'set' you mean 'some other value than 0?

Yes, since the back-end can't have an actual bucket with an id of 0.

I see it as bad style ff a possible value of a variable is 0

Wait, are you saying that the value of oldBucket could be 0? No, it's either a bucket object, or undefined (the latter shouldn't happen). But ok, I'll update it to != undefined to cover null and undefined, but not 0.

> By 'set' you mean 'some other value than 0? Yes, since the back-end can't have an actual bucket with an `id` of 0. > I see it as bad style ff a possible value of a variable is 0 Wait, are you saying that the value of `oldBucket` could be `0`? No, it's either a bucket object, or undefined (the latter shouldn't happen). But ok, I'll update it to `!= undefined` to cover `null` and `undefined`, but not 0.
konrad commented 2023-03-28 10:02:56 +00:00
Owner

Even if you set it to 0 the api will set it to the default bucket.

Even if you set it to `0` the api will set it to the default bucket.
dpschen commented 2023-03-28 10:06:00 +00:00
Member

Wait, are you saying that the value of oldBucket could be 0?

The initial set value in the frontend of bucketId is 0.

So even if that might be replaced in the moment the API answers it currently is true for short period of time. If this is not wished, we should adjust the initial value in the model (e.g. as you said @WofWca to undefined).

Note I talk here about the bucketId and not the oldBucket. But since the latter is based on the former I thought not relying on implicit type conversion would be clearer.

> Wait, are you saying that the value of oldBucket could be 0? The initial set value in the frontend of `bucketId` is `0`. So even if that might be replaced in the moment the API answers it currently is true for short period of time. If this is not wished, we should adjust the initial value in the model (e.g. as you said @WofWca to `undefined`). Note I talk here about the `bucketId` and not the `oldBucket`. But since the latter is based on the former I thought not relying on implicit type conversion would be clearer.
WofWca commented 2023-03-28 10:08:40 +00:00
Author
Contributor

The oldBucket variable is the return value of the find function that is called on the array of bucket objects. Are you taking about oldBucket or task.bucketId?

Edit: saw the edit.

The `oldBucket` variable is the return value of the `find` function that is called on the array of bucket objects. Are you taking about `oldBucket` or `task.bucketId`? Edit: saw the edit.
👍 1
dpschen commented 2023-03-28 10:12:27 +00:00
Member

Either way, I don't care anymore :D This is way too much effort for way to few impact. Change as you wish ;)

Either way, I don't care anymore :D This is way too much effort for way to few impact. Change as you wish ;)
😆 1
dpschen commented 2023-03-23 17:03:05 +00:00
Member
Copy Link

It already is marked as done if it's moved into the done bucket on
the backend:
6aadaaaffc/pkg/models/tasks.go (L867-L869)

Can we do this in the frontend as well in this pr?
That way we don't have to wait for the answer of the backend.

> It already is marked as done if it's moved into the done bucket on the backend: 6aadaaaffc/pkg/models/tasks.go (L867-L869) Can we do this in the frontend as well in this pr? That way we don't have to wait for the answer of the backend.
dpschen requested review from dpschen 2023-03-23 17:03:18 +00:00
WofWca commented 2023-03-23 17:53:55 +00:00
Author
Contributor
Copy Link

Can we do this in the frontend as well in this pr?

It is already this way, if you look closely. It's the reason I opened the MR actually, to make it easier to implement the offline version 😉

> Can we do this in the frontend as well in this pr? It is already this way, if you look closely. It's the reason I opened the MR actually, to make it easier to implement the offline version 😉
dpschen commented 2023-03-23 18:23:57 +00:00
Member
Copy Link

Can we do this in the frontend as well in this pr?

It is already this way, if you look closely. It's the reason I opened the MR actually, to make it easier to implement the offline version 😉

From the title I judged that this pr is about moving from the bucket, while you mention that it's marked as done when moving into the bucket. So I assumed that this doesn't currently happen in the frontend and isn't yet included in this pr.

> > Can we do this in the frontend as well in this pr? > > It is already this way, if you look closely. It's the reason I opened the MR actually, to make it easier to implement the offline version 😉 From the title I judged that this pr is about moving _from_ the bucket, while you mention that it's marked as done when moving _into_ the bucket. So I assumed that this doesn't currently happen in the frontend and isn't yet included in this pr.
WofWca commented 2023-03-23 18:28:46 +00:00
Author
Contributor
Copy Link

Well, yeah, this MR is about moving from the bucket, since moving into the bucket already works on the main branch, but it works thanks to the backend.

Well, yeah, this MR is about moving _from_ the bucket, since moving _into_ the bucket already works on the `main` branch, but it works thanks to the backend.
dpschen commented 2023-03-23 18:56:03 +00:00
Member
Copy Link

Well, yeah, this MR is about moving from the bucket, since moving into the bucket already works on the main branch, but it works thanks to the backend.

Yes. So with that in mind I thought it makes sense to do this additionally in the frontend in the sense of optimistic UI. We don't have to implement the full optimistic UI pattern here already, but I think it makes sense to align in the frontend what happens in both move directions.

> Well, yeah, this MR is about moving _from_ the bucket, since moving _into_ the bucket already works on the `main` branch, but it works thanks to the backend. Yes. So with that in mind I thought it makes sense to do this _additionally_ in the frontend in the sense of [optimistic UI](https://www.smashingmagazine.com/2016/11/true-lies-of-optimistic-user-interfaces/). We don't have to implement the full optimistic UI pattern here already, but I think it makes sense to align in the frontend what happens in both move directions.
dpschen commented 2023-03-27 21:20:41 +00:00
Member
Copy Link

Well, yeah, this MR is about moving from the bucket, since moving into the bucket already works on the main branch, but it works thanks to the backend.

Yes. So with that in mind I thought it makes sense to do this additionally in the frontend in the sense of optimistic UI. We don't have to implement the full optimistic UI pattern here already, but I think it makes sense to align in the frontend what happens in both move directions.

@WofWca:
I think I didn't comprehend that this part of the code…

		newBucket.isDoneBucket !== oldBucket.isDoneBucket
	) {
		newTask.done = newBucket.isDoneBucket

…does change the done state in both moving directions. So all good here, my bad for misunderstanding!

> > Well, yeah, this MR is about moving _from_ the bucket, since moving _into_ the bucket already works on the `main` branch, but it works thanks to the backend. > > Yes. So with that in mind I thought it makes sense to do this _additionally_ in the frontend in the sense of [optimistic UI](https://www.smashingmagazine.com/2016/11/true-lies-of-optimistic-user-interfaces/). We don't have to implement the full optimistic UI pattern here already, but I think it makes sense to align in the frontend what happens in both move directions. @WofWca: I think I didn't comprehend that this part of the code… ```ts newBucket.isDoneBucket !== oldBucket.isDoneBucket ) { newTask.done = newBucket.isDoneBucket ``` …does change the done state in both moving directions. So all good here, my bad for misunderstanding!
👍 1
WofWca force-pushed mark-undone-after-moving-from-done-bucket from b83366f410 to b082e81629 2023-03-28 10:05:42 +00:00 Compare
WofWca commented 2023-03-28 10:11:39 +00:00
Author
Contributor
Copy Link

So all good here, my bad for misunderstanding!

Perhaps I wasn't good at communicating this.

So, is this ready for merging now?

> So all good here, my bad for misunderstanding! Perhaps I wasn't good at communicating this. So, is this ready for merging now?
dpschen approved these changes 2023-03-28 10:12:33 +00:00
konrad approved these changes 2023-03-28 10:14:33 +00:00
konrad scheduled this pull request to auto merge when all checks succeed 2023-03-28 10:14:48 +00:00
konrad commented 2023-03-28 10:15:05 +00:00
Owner
Copy Link

We should do a follow-up at some point to change this in the api as well.

We should do a follow-up at some point to change this in the api as well.
konrad merged commit 30adad5ae6 into main 2023-03-28 10:21:23 +00:00
WofWca deleted branch mark-undone-after-moving-from-done-bucket 2023-03-29 07:01:19 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
dpschen
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
4 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#3291
No description provided.
Delete Branch "WofWca/frontend:mark-undone-after-moving-from-done-bucket"

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?