Skip to content

Notifications: Notification popup appeared, but zero in list screen #415

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

Merged
merged 9 commits into from
Jul 29, 2025

Conversation

swoocn
Copy link
Member

@swoocn swoocn commented Jul 17, 2025

FE adjustments for proper integration with amended getNotifications endpoint.

[see commits]

@adisa39
Copy link
Collaborator

adisa39 commented Jul 21, 2025

Added notification button to sidebar.

@@ -417,6 +417,24 @@ function Sidebar(props: any) {
</div>
)}

<div className="mb-2">
<Link href={`/${locale}/notification`}>
Copy link
Member Author

Choose a reason for hiding this comment

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

👍

@@ -97,42 +104,44 @@ export default function NotificationPage() {
}, [currentUser?.pi_uid]);

useEffect(() => {
if (observer.current) observer.current.disconnect();
if (loadMoreObserver.current) loadMoreObserver.current.disconnect();
if (!currentUser?.pi_uid || !hasMore) return;
Copy link
Member Author

Choose a reason for hiding this comment

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

Nice @adisa39!
A refactor suggestion.. what are your thoughts about extracting this behavior into a custom hook so it can be reusable in any list-based screens? I figure we could call it useInfiniteScroll.ts and place it into a /hooks folder.

Is that something you want to try coding, or would you prefer that I do it? Let me know!

Copy link
Member Author

@swoocn swoocn left a comment

Choose a reason for hiding this comment

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

The PR changes look good @adisa39! Good job! 👍
I left a comment suggesting a possible refactoring opportunity that may be considered. But for the sake of time, I'm totally fine with revisiting it later, if you feel so. Feel free to approve so I can proceed with the merge. Thanks again!

Copy link
Member

Choose a reason for hiding this comment

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

Solid cleanup 👍

const response = await axiosClient.get(`/notifications/${pi_uid}?skip=${skip}&limit=${limit}`, {
headers,

const queryParams = new URLSearchParams({
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@DarinHajou DarinHajou left a comment

Choose a reason for hiding this comment

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

LGTM! Just a small naming issue at 'show={setShowNotificationPopup}'

@swoocn
Copy link
Member Author

swoocn commented Jul 25, 2025

One thing I also noticed — the text in the input fields doesn’t overflow or become scrollable, which prevents users from seeing the entire content if it's too long. I'll plan to look into this when I get a chance, unless someone else gets to it first.

We may want to implement this in our other existing input fields as well.

@swoocn
Copy link
Member Author

swoocn commented Jul 26, 2025

One thing I also noticed — the text in the input fields doesn’t overflow or become scrollable, which prevents users from seeing the entire content if it's too long. I'll plan to look into this when I get a chance, unless someone else gets to it first.

We may want to implement this in our other existing input fields as well.

Nevermind; user error. Looks like the fields are scrollable.. though may not be intuitive for users.
Perhaps in the future we can emit a tooltip instead.

@swoocn
Copy link
Member Author

swoocn commented Jul 26, 2025

I went ahead and extracted the pagination-related useEffect into its own custom hook and tested it with the Notification feature, which appears to be working well. We can plan to reuse this hook for the other pagination-driven pages and regression test when we get to the technical debt cleanup task @ https://sharing.clickup.com/9015444408/t/h/86c4p227d/KKLUOY257LSYH0K

@swoocn swoocn requested a review from DarinHajou July 26, 2025 21:02
@swoocn
Copy link
Member Author

swoocn commented Jul 27, 2025

Hi @DarinHajou - it appears the PR is locked due to your requested change(s), which I believe have been addressed now. Could you please re-review when you can? Thanks for your contribution!

Copy link
Member

@DarinHajou DarinHajou left a comment

Choose a reason for hiding this comment

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

Approved after talking to Danny

@swoocn swoocn merged commit 05b8c54 into dev Jul 29, 2025
1 check passed
@swoocn swoocn deleted the bug/fix-notifications branch July 29, 2025 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants