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

wip: feat: abstract NotificationItem in dedicated component #2681

Closed
dpschen wants to merge 6 commits from dpschen/frontend:feature/abstract-NotificationItem-component into main
pull from: dpschen/frontend:feature/abstract-NotificationItem-component
merge into: vikunja:main
Conversation 15 Commits 6 Files Changed 12 +490 -316
dpschen commented 2022-11-11 21:28:38 +00:00
Member
Copy Link
No description provided.
dpschen added the
area/internal-code
 label 2022-11-11 21:28:38 +00:00
konrad was assigned by dpschen 2022-11-11 21:28:38 +00:00
dpschen force-pushed feature/abstract-NotificationItem-component from 22155d1e14 to c6ff4e4dee 2022-11-11 22:29:43 +00:00 Compare
frederick commented 2022-11-11 22:33:40 +00:00
Member
Copy Link

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://2681-feature-abstract-notificationite--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 dpschen! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://2681-feature-abstract-notificationite--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.
konrad reviewed 2022-11-12 10:57:31 +00:00
src/components/notifications/NotificationItem.vue Outdated
@ -0,0 +64,4 @@
// Nothing
break
case NOTIFICATION_NAMES.LIST_CREATED:
to.name = 'task.index'
konrad commented 2022-11-12 10:55:01 +00:00
Owner

Shouldn't this be list.index? (not sure if the route is actually called that)

Shouldn't this be `list.index`? (not sure if the route is actually called that)
dpschen commented 2022-11-12 13:30:31 +00:00
Author
Member

This name is correct.

This name is correct.
👍 1
dpschen marked this conversation as resolved
src/components/notifications/NotificationItem.vue Outdated
@ -0,0 +83,4 @@
switch (props.notification.name) {
case NOTIFICATION_NAMES.TASK_COMMENT:
return `commented on ${getTextIdentifier(notification.task)}`
konrad commented 2022-11-12 10:56:06 +00:00
Owner

I wonder if we can translate these but that's for another PR.

I wonder if we can translate these but that's for another PR.
dpschen commented 2022-11-12 13:30:46 +00:00
Author
Member

Yes, saw that too!

Yes, saw that too!
dpschen commented 2022-11-14 21:10:03 +00:00
Author
Member

I added translations

I added translations
src/components/notifications/notifications.vue Outdated
@ -54,4 +36,1 @@
import BaseButton from '@/components/base/BaseButton.vue'
import User from '@/components/misc/user.vue'
import { NOTIFICATION_NAMES as names, type INotification} from '@/modelTypes/INotification'
import {closeWhenClickedOutside} from '@/helpers/closeWhenClickedOutside'
konrad commented 2022-11-12 10:56:54 +00:00
Owner

If vueuse has a helper like ours, do we even need ours?

If vueuse has a helper like ours, do we even need ours?
dpschen commented 2022-11-12 13:33:08 +00:00
Author
Member

No, I'm replacing ours piece by piece.
Not all vueuse composables are perfect though.
E.g. there is one useTextareaAutosize, but it e.g. doesn't cover the case where the textarea scales with the width of the window. So we should be careful. The onClickoutside seems well written though!

No, I'm replacing ours piece by piece. Not all vueuse composables are perfect though. E.g. there is one `useTextareaAutosize`, but it e.g. doesn't cover the case where the textarea scales with the width of the window. So we should be careful. The `onClickoutside` seems well written though!
👍 1
dpschen marked this conversation as resolved
konrad requested changes 2022-11-12 10:59:46 +00:00
konrad left a comment
Owner

Looks like this broke the notifications :/

image

On current main:

image

Looks like this broke the notifications :/ ![image](/attachments/786874f2-7a78-48c1-ae4a-bd7d9a9ba1ff) On current main: ![image](/attachments/1e47d83a-4381-4242-be99-5756bf729445)
image.png
36 KiB
image.png
9.7 KiB
dpschen commented 2022-11-12 13:35:16 +00:00
Author
Member
Copy Link

Tricky to debug, if you don't have notifications. Histoire would help here 🤔

Tricky to debug, if you don't have notifications. Histoire would help here 🤔
dpschen force-pushed feature/abstract-NotificationItem-component from c6ff4e4dee to 2f576e5006 2022-11-12 13:39:07 +00:00 Compare
dpschen commented 2022-11-12 13:39:45 +00:00
Author
Member
Copy Link

The import of the new component was missing. I hope that was it.

The import of the new component was missing. I hope that was it.
dpschen requested review from konrad 2022-11-12 13:39:48 +00:00
konrad commented 2022-11-12 20:31:10 +00:00
Owner
Copy Link

Now there's this message:

image

and it uses up all cpu ressource, making the browser unusable.

Now there's this message: ![image](/attachments/93781941-747d-437e-a381-7bf6dab49803) and it uses up all cpu ressource, making the browser unusable.
image.png
6.8 KiB
konrad commented 2022-11-12 20:32:21 +00:00
Owner
Copy Link

Can you add a cypress test for this?

Can you add a cypress test for this?
dpschen force-pushed feature/abstract-NotificationItem-component from 2f576e5006 to d99441d72e 2022-11-13 17:49:17 +00:00 Compare
konrad commented 2022-11-13 20:50:57 +00:00
Owner
Copy Link

Now it seems to kind of work (at least it does not crash) but marking notifications as read seems broken.

Now it seems to kind of work (at least it does not crash) but marking notifications as read seems broken.
dpschen force-pushed feature/abstract-NotificationItem-component from d99441d72e to 83297039a9 2022-11-13 20:56:34 +00:00 Compare
konrad commented 2022-11-13 21:45:57 +00:00
Owner
Copy Link

Can you fix the lint errors?

Can you fix the lint errors?
👍 1
dpschen force-pushed feature/abstract-NotificationItem-component from 83297039a9 to 8969ddd831 2022-11-14 13:08:45 +00:00 Compare
dpschen commented 2022-11-14 13:11:27 +00:00
Author
Member
Copy Link

I fixed the lint errors.

Regarding the cypress tests:
Do you really think that's necessary here.
I think it would help more if we would have historie to test these kind of components in isolation. This way I could test all the message types without the need to manipulating anything in the backend.

I fixed the lint errors. Regarding the cypress tests: Do you really think that's necessary here. I think it would help more if we would have historie to test these kind of components in isolation. This way I could test all the message types without the need to manipulating anything in the backend.
dpschen force-pushed feature/abstract-NotificationItem-component from c737242627 to e66a8c69f3 2022-11-14 14:34:36 +00:00 Compare
konrad commented 2022-11-14 16:06:08 +00:00
Owner
Copy Link

Do you really think that's necessary here.
I think it would help more if we would have historie to test these kind of components in isolation. This way I could test all the message types without the need to manipulating anything in the backend.

I think having more tests is always a good thing. In this PR it would have helped because you wouldn't need to first create actions which led to notifitications to see if the notification component works. Having a test would automate that and yield faster feedback. Sure, histoire would be nice but we don't have it right now.

> Do you really think that's necessary here. I think it would help more if we would have historie to test these kind of components in isolation. This way I could test all the message types without the need to manipulating anything in the backend. I think having more tests is always a good thing. In this PR it would have helped because you wouldn't need to first create actions which led to notifitications to see if the notification component works. Having a test would automate that and yield faster feedback. Sure, histoire would be nice but we don't have it right now.
dpschen force-pushed feature/abstract-NotificationItem-component from e66a8c69f3 to 9fe0b5b766 2022-11-14 21:06:30 +00:00 Compare
dpschen commented 2022-11-15 17:22:33 +00:00
Author
Member
Copy Link

Since cypress still doesn't work for me locally (I also didn't try anymore to fix that though) the feedbackloop for me is insane long. That's mostly why I try to avoid writing cypress tests right now.
I guess I could almost add histoire in the time I wait for cypress to run through :D

Since cypress still doesn't work for me locally (I also didn't try anymore to fix that though) the feedbackloop for me is insane long. That's mostly why I try to avoid writing cypress tests right now. I guess I could almost add histoire in the time I wait for cypress to run through :D
👍 1
dpschen commented 2022-11-15 17:23:10 +00:00
Author
Member
Copy Link

Also: this should all work now.

Also: this should all work now.
konrad commented 2022-11-16 15:08:28 +00:00
Owner
Copy Link

Looks like no notifications are show now :/

Looks like no notifications are show now :/
😕 1
dpschen commented 2022-11-16 16:30:52 +00:00
Author
Member
Copy Link

Also: this should all work now.

I was lying 

> Also: this should all work now. I was lying 
dpschen force-pushed feature/abstract-NotificationItem-component from 9fe0b5b766 to 2581c88626 2023-01-04 16:40:10 +00:00 Compare
konrad commented 2023-01-05 10:22:55 +00:00
Owner
Copy Link

Still no notifications :/

Still no notifications :/
dpschen commented 2023-01-05 13:34:09 +00:00
Author
Member
Copy Link

Still no notifications :/

Yes. I rebased this so that I could debug it better.
Still on this. I might need your help for realistic notification example data.

> Still no notifications :/ Yes. I rebased this so that I could debug it better. Still on this. I might need your help for realistic notification example data.
👍 1
dpschen changed title from feat: abstract NotificationItem in dedicated component to wip: feat: abstract NotificationItem in dedicated component 2023-01-05 13:34:59 +00:00
dpschen force-pushed feature/abstract-NotificationItem-component from 2581c88626 to c0ca76fccf 2023-01-06 13:05:15 +00:00 Compare
konrad closed this pull request 2024-02-07 10:45:08 +00:00
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
konrad
3 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#2681
No description provided.
Delete Branch "dpschen/frontend:feature/abstract-NotificationItem-component"

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?