- Notifications
You must be signed in to change notification settings - Fork907
feat: add load more notifications on inbox#17030
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -18,7 +18,7 @@ export const ScrollArea = React.forwardRef< | |||
| <ScrollAreaPrimitive.Viewport className="h-full w-full rounded-[inherit]"> | |||
| {children} | |||
| </ScrollAreaPrimitive.Viewport> | |||
| <ScrollBar /> | |||
| <ScrollBarclassName="z-10"/> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It was getting behind the sticky header.
| className={cn([ | ||
| "[--bottom-offset:48px]", | ||
| "[--max-height:calc(var(--radix-popover-content-available-height)-var(--bottom-offset))]", | ||
| "[&>[data-radix-scroll-area-viewport]]:max-h-[var(--max-height)]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Tried to make it easier to read the classes and reasoning about their values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do you know what happens if the result of--max-height falls below 0? We're always subtracting 48 no matter what, so I don't know if it makes sense to do a CSS clamp to make sure it can't fall below 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
To make that happen, the screen height would be 48px or less than that. Since this is not a real scenario, even it is possible, I think it is ok to not care about that. Makes sense?
| getErrorDetail(error), | ||
| ); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I just found way easier to use a mutation to update the data than use theuseInfiniteQuery and its pages abstraction. Mostly when trying to update the previous query data.
Parkreiner left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Going to be approving to make sure you're not blocked, but I can't help but feel like theuseInfiniteQuery approach would be simpler
I'm going to try taking a crack at refactoring the code to use it later today, and if I can figure something out, open up a second PR with you tagged
| @@ -41,12 +47,18 @@ export const InboxPopover: FC<InboxPopoverProps> = ({ | |||
| <PopoverTrigger asChild> | |||
| <InboxButton unreadCount={unreadCount} /> | |||
| </PopoverTrigger> | |||
| <PopoverContent className="w-[466px]" align="end"> | |||
| <PopoverContent className="w-[466px] flex flex-col" align="end"> | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just now noticing: do we want the width to be hard-coded, or could we do something likew-full max-w-[466px]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think what you proposed is way better. thx
| className={cn([ | ||
| "[--bottom-offset:48px]", | ||
| "[--max-height:calc(var(--radix-popover-content-available-height)-var(--bottom-offset))]", | ||
| "[&>[data-radix-scroll-area-viewport]]:max-h-[var(--max-height)]", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do you know what happens if the result of--max-height falls below 0? We're always subtracting 48 no matter what, so I don't know if it makes sense to do a CSS clamp to make sure it can't fall below 0
| const lastNotification = | ||
| inboxRes.notifications[inboxRes.notifications.length - 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
We want to make sure thatnotifications isn't empty. Otherwise,lastNotification.id will throw an error on the next line
I definitely see value on this. I think I can learn a lot from this 🙏 |
1593861 intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Users need to see older notifications, so to make that happen, we added a load more button at the end of the notifications list.
Demo:
Screen.Recording.2025-03-20.at.14.51.07.mov
What is missing?
As you can notice, I didn't add tests for this feature. I tried, but I didn't find a good solution for testing scroll events. However I was able to get it working, but it was too cumbersome that I decided to remove because of its maintenence burden.