Skip to content

#509: Add Notification Bell and Notification Page#599

Open
BauZee wants to merge 11 commits intodevelopfrom
origin/feature/notification-bell-v2
Open

#509: Add Notification Bell and Notification Page#599
BauZee wants to merge 11 commits intodevelopfrom
origin/feature/notification-bell-v2

Conversation

@BauZee
Copy link
Copy Markdown
Collaborator

@BauZee BauZee commented Apr 13, 2026

image image

@cloudflare-workers-and-pages
Copy link
Copy Markdown
Contributor

cloudflare-workers-and-pages bot commented Apr 13, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
aura-historia-staging 8450330 Commit Preview URL

Branch Preview URL
Apr 15 2026, 06:06 PM

@BauZee BauZee requested a review from lfranke42 April 13, 2026 20:57
@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
61.6% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@BauZee
Copy link
Copy Markdown
Collaborator Author

BauZee commented Apr 13, 2026

Note:

I created a completely new branch based on the current develop branch to have a fresh, clean starting point, and moved my changes there.
The old branch had accumulated so many merge conflicts by then that they were no longer easy to resolve. I was simply worried that, with so many conflicts, I might accidentally overlook something or unintentionally break the code. That’s why I took this approach. I’ll close the old branch once this one is merged

Comment thread src/components/common/Header.tsx Outdated
{isLoginEnabled && user && (
<div
className={cn(
isFloating ? "bg-card backdrop-blur-sm rounded-xl p-1 shadow-sm" : "",
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No rounded corners please. Also bg color doesn't match to rest of the elements here

{!seen && (
<Tooltip>
<TooltipTrigger asChild>
<button
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use shadcn button here?

Comment on lines +49 to +81
<Tooltip>
<TooltipTrigger asChild>
<button
type="button"
aria-label={t("notifications.markRead")}
disabled={markAsSeen.isPending}
onClick={() =>
markAsSeen.mutate(notification.originEventId)
}
className="shrink-0 rounded p-1 text-muted-foreground transition-colors hover:text-primary"
>
<Check className="size-3" />
</button>
</TooltipTrigger>
<TooltipContent>{t("notifications.markRead")}</TooltipContent>
</Tooltip>
)}
<Tooltip>
<TooltipTrigger asChild>
<button
type="button"
aria-label={t("notifications.delete")}
disabled={deleteNotification.isPending}
onClick={() =>
deleteNotification.mutate(notification.originEventId)
}
className="-mr-1 shrink-0 rounded p-1 text-muted-foreground transition-colors hover:text-destructive"
>
<Trash2 className="size-3" />
</button>
</TooltipTrigger>
<TooltipContent>{t("notifications.delete")}</TooltipContent>
</Tooltip>
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest moving the tick and trash can buttons to the bottom right of the notificiation items. That way the top left isn't as crowded anymore.
Also I'd suggest using a single tick button for marking single notifications as read, double tick only for marking all notifications as read.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

I'd rather move the time down. That way, it'll be consistent with the notification page.
Although I didn't think it looked too cluttered when everything was in the top-right corner...

{!seen && (
<Tooltip>
<TooltipTrigger asChild>
<button
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use shadcn button here?

Comment thread src/routes/_auth.notifications.tsx Outdated
import { NotificationResults } from "@/components/notification/NotificationResults.tsx";
import { generatePageHeadMeta } from "@/lib/seo/pageHeadMeta.ts";

export const Route = createFileRoute("/_auth/notifications")({
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add the route to vite config, so we exclude it for prerendering. Maybe we should put all of the authenticated routes behind a proper route as well so we can just blacklist everything that comes under that?
Something like \_auth\me\notifications?
Then we can just exclude \me\

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like that, I'll do it

@BauZee BauZee requested a review from lfranke42 April 15, 2026 18:26
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.

2 participants