vikunja/vikunja
5
48
Fork 86
Code Issues 150 Pull Requests 37 Packages Releases 37 Activity

WIP: feat: use withDefaults in Description #2453

Draft
dpschen wants to merge 1 commits from dpschen/vikunja:feat/description-use-withDefaults into main
pull from: dpschen/vikunja:feat/description-use-withDefaults
merge into: vikunja:main
Conversation 1 Commits 1 Files Changed 1 +17 -23
dpschen commented 2024-06-20 10:20:10 +00:00
Member
Copy Link

This will not fix the current issues yet, but I think it makes sense to start with this to rule this out.

This will not fix the current issues yet, but I think it makes sense to start with this to rule this out.
👍 1
dpschen added 1 commit 2024-06-20 10:20:15 +00:00
feat: use withDefaults in Description
Some checks failed
continuous-integration/drone/pr Build is failing
Details
16da189510
dpschen force-pushed feat/description-use-withDefaults from 16da189510 to 277344d216 2024-06-26 10:28:28 +00:00 Compare
dpschen reviewed 2024-06-26 10:30:19 +00:00
frontend/src/components/input/editor/TipTap.vue Outdated
@ -548,3 +542,3 @@
}
uploadAndInsertFiles(files)
uploadAndInsertFiles(files) // TODO: await?
dpschen commented 2024-06-26 10:30:19 +00:00
Author
Member
Copy Link

@konrad Shouldn't we await here?

@konrad Shouldn't we await here?
konrad commented 2024-06-28 21:10:15 +00:00
Owner
Copy Link

In general yes, in this specific case I think it's fine to omit it because there isn't anything happening after the call to uploadAndInsertFiles. It's fine if that happens asynchronously.

In general yes, in this specific case I think it's fine to omit it because there isn't anything happening after the call to `uploadAndInsertFiles`. It's fine if that happens asynchronously.
dpschen commented 2024-07-04 19:15:12 +00:00
Author
Member
Copy Link

Since addImage is async shouldn't we still wait?

Since `addImage` is async shouldn't we still wait?
dpschen reviewed 2024-06-26 10:33:03 +00:00
frontend/src/components/input/editor/TipTap.vue Outdated
@ -586,4 +582,2 @@
input?.removeEventListener('paste', handleImagePaste)
}
})
if (editShortcut !== '') {
dpschen commented 2024-06-26 10:33:03 +00:00
Author
Member
Copy Link

Since we don't remove the eventListener, when editShortcut changes (e.g. by watching the value), we should always try to remove it during onBeforeUnmount – since in theory it could be that the value of editShourtcut has changed in between.

Since we don't remove the eventListener, when editShortcut changes (e.g. by watching the value), we should always try to remove it during onBeforeUnmount – since in theory it could be that the value of editShourtcut has changed in between.
👍 1
konrad commented 2024-07-05 12:05:37 +00:00
Owner
Copy Link

But won't the shortcut never change because we don't watch the shortcut value?

But won't the shortcut never change because we don't watch the shortcut value?
dpschen reviewed 2024-06-26 10:34:28 +00:00
frontend/src/components/tasks/partials/Description.vue Outdated
@ -47,3 +47,3 @@
import {useTaskStore} from '@/stores/tasks'
type AttachmentUploadFunction = (file: File, onSuccess: (attachmentUrl: string) => void) => Promise<string>
export type AttachmentUploadFunction = (file: File, onSuccess: (attachmentUrl: string) => void) => Promise<string>
dpschen commented 2024-06-26 10:34:28 +00:00
Author
Member
Copy Link

Since we return a promise here: why do we even have the onSuccess callback? if it fails we could reject instead.

Since we return a promise here: why do we even have the `onSuccess` callback? if it fails we could reject instead.
konrad commented 2024-07-05 12:03:52 +00:00
Owner
Copy Link

I guess that's old and grown. Not sure how it is used, but yeah I guess we could use a promise.

I guess that's old and grown. Not sure how it is used, but yeah I guess we could use a promise.
dpschen reviewed 2024-06-26 10:37:16 +00:00
frontend/src/components/input/editor/TipTap.vue Outdated
@ -581,4 +578,3 @@
onBeforeUnmount(() => {
nextTick(() => {
if (typeof uploadCallback !== 'undefined') {
dpschen commented 2024-06-26 10:37:16 +00:00
Author
Member
Copy Link

uploadCallback might be undefined now, but it changed during runtime. So we should always try to remove the eventListener.

I'm unsure but I think the `nextTick is a mistake here.

`uploadCallback` might be undefined now, but it changed during runtime. So we should always try to remove the eventListener. I'm unsure but I think the `nextTick is a mistake here.
konrad commented 2024-07-05 12:04:53 +00:00
Owner
Copy Link

Does it work without the nextTick? Either there was some issue with it or I was frustrated and that kind of fixed it 😅

Does it work without the `nextTick`? Either there was some issue with it or I was frustrated and that kind of fixed it 😅
dpschen force-pushed feat/description-use-withDefaults from 277344d216 to 184c2dbe4e 2024-07-05 10:48:36 +00:00 Compare
dpschen force-pushed feat/description-use-withDefaults from 184c2dbe4e to c279a20c96 2024-07-05 11:01:33 +00:00 Compare
dpschen force-pushed feat/description-use-withDefaults from c279a20c96 to 0f72421695 2024-07-05 11:31:13 +00:00 Compare
dpschen force-pushed feat/description-use-withDefaults from 0f72421695 to 9bdfdb9452 2024-07-18 17:13:12 +00:00 Compare
dpschen force-pushed feat/description-use-withDefaults from 9bdfdb9452 to 490e29a7fa 2024-07-18 17:17:30 +00:00 Compare
frederick commented 2024-07-18 17:26:08 +00:00
Member
Copy Link

Hi dpschen!

Thank you for creating a PR!

I've deployed the frontend changes of this PR on a preview environment under this URL: https://2453-feat-description-use-withdefault--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 somewhere. The easiest to use is https://try.vikunja.io/.

This preview does not contain any changes made to the api, only the frontend.

Have a nice day!

Beep boop, I'm a bot.

Hi dpschen! Thank you for creating a PR! I've deployed the frontend changes of this PR on a preview environment under this URL: https://2453-feat-description-use-withdefault--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 somewhere. The easiest to use is https://try.vikunja.io/. This preview does not contain any changes made to the api, only the frontend. Have a nice day! > Beep boop, I'm a bot.
All checks were successful
continuous-integration/drone/pr Build is passing
Required
Details
This pull request is marked as a work in progress.
This branch is out-of-date with the base branch
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u feat/description-use-withDefaults:dpschen-feat/description-use-withDefaults
git checkout dpschen-feat/description-use-withDefaults
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
Clear labels
dependencies
duplicate
This issue or pull request already exists
help wanted
Need some help
invalid
Something is wrong
kind/bug
Something is not working
kind/feature
New feature
needs reproduction
question
More information is needed
security
wontfix
This won't be fixed
No Label
Milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: vikunja/vikunja#2453
No description provided.
Delete Branch "dpschen/vikunja:feat/description-use-withDefaults"

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?