Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add notifications UI #1

Open
wants to merge 102 commits into
base: development
Choose a base branch
from
Open

feat: add notifications UI #1

wants to merge 102 commits into from

Conversation

zugdev
Copy link
Collaborator

@zugdev zugdev commented Dec 7, 2024

@zugdev zugdev requested a review from 0x4007 as a code owner December 7, 2024 17:40
@zugdev zugdev marked this pull request as draft December 7, 2024 17:43
@zugdev
Copy link
Collaborator Author

zugdev commented Dec 7, 2024

@rndquu could you please increase the auth token expiry date in supabase?

@0x4007
Copy link
Member

0x4007 commented Dec 8, 2024

@zugdev pull tag here

@zugdev
Copy link
Collaborator Author

zugdev commented Jan 8, 2025

The only notifications that matter for this view are the ones associated with tasks that have priority levels. Otherwise I think the classic notifications UI should be used for everything else.

@0x4007 I could hide mention with no comments, or specific types and still allow review requested, either whitelist or blacklist types if thats interesting to you. rn I have hidden the no comment notifs

@0x4007
Copy link
Member

0x4007 commented Jan 8, 2025

I just want the notifications view that GitHub provides but to exclude the following:

  • notifications not associated with a priority label
  • draft pulls
  • UbiquityOS interactions (bot commands and replies)

That would cut out almost all of the noise.

@zugdev
Copy link
Collaborator Author

zugdev commented Jan 8, 2025

I just want the notifications view that GitHub provides but to exclude the following:

  • notifications not associated with a priority label
  • draft pulls
  • UbiquityOS interactions (bot commands and replies)

That would cut out almost all of the noise.

lmk when u test it out

@0x4007
Copy link
Member

0x4007 commented Jan 8, 2025

Confused why there's so many now.

image

image

You can see the scroll bar on the right to approximate how many.

Also when the comment loads in sync it jitters the row and looks bad. set the row height to be static so this doesn't happen.

@zugdev
Copy link
Collaborator Author

zugdev commented Jan 8, 2025

@0x4007 instead of fixed height I totally fixed jitters by prefetching comments and then rendering

about the inbox message size. yes, it is weird, but if u try the api yourself sometimes in github stuff is marked as read but the api doesnt say so.

also, I've diffed upstream and there is nothing different about the title

@0x4007
Copy link
Member

0x4007 commented Jan 8, 2025

Posting a shortcut to the deploys because GitHub iOS client makes it extremely tedious to get to

#1 (comment)

@0x4007
Copy link
Member

0x4007 commented Jan 8, 2025

@0x4007 instead of fixed height I totally fixed jitters by prefetching comments and then rendering

That should be fine

about the inbox message size. yes, it is weird, but if u try the api yourself sometimes in github stuff is marked as read but the api doesnt say so.

This is obviously a problem that needs to be fixed for this tool to be useful. Also these just started all loading in since the last few changes you made so I'm inclined to believe that the problem was introduced based on recent changes.

also, I've diffed upstream and there is nothing different about the title

The html order matters for the checkbox which is clearly rendering. Look for any css code selectors using ~ for a clue.

image

@0x4007
Copy link
Member

0x4007 commented Jan 8, 2025

9520dfc is the last good one

@0x4007
Copy link
Member

0x4007 commented Jan 8, 2025

5c95353 loads too many notifications

@0x4007
Copy link
Member

0x4007 commented Jan 8, 2025

You need to review your work before requesting reviews again

@zugdev
Copy link
Collaborator Author

zugdev commented Jan 9, 2025

alright, mb. all should be fixed rn

@0x4007
Copy link
Member

0x4007 commented Jan 9, 2025

image

Looks good I just wish I had more notifications, but I just finished reviewing everything! @gentlementlegen @whilefoo @rndquu @EresDev can you guys take a look at the latest deploy and offer feedback based on your notifications?

https://4626f4b9.notifications-ubq-fi.pages.dev/

@0x4007
Copy link
Member

0x4007 commented Jan 9, 2025

In other news, I wanted to share that the noise from the bot will be mostly mitigated once this is implemented

ubiquity-os/plugins-wishlist#54

@0x4007
Copy link
Member

0x4007 commented Jan 9, 2025

image

Is the caching implemented correctly?

image

@zugdev
Copy link
Collaborator Author

zugdev commented Jan 9, 2025

yup, u can check console to better find the reasons but in approval/funding UI there isnt a comment, instead its a requested change. all the closed PRs and issues dont show. also try toggling show bot. perhaps we should switch the show bot toggle for show all (related to ubiquity of course)

@zugdev
Copy link
Collaborator Author

zugdev commented Jan 9, 2025

alright I've added logs on every notification hiden, u can open console to see the reason, this will allow us to finetune and decide what should go through over time as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom Notifications Viewer
4 participants