881 notifications with paging#901
Conversation
… cause 'isLoading' to flash on the NotificationsSection
github-actions
Bot
added
FY26 Sprint 10
jp-tosca
left a comment
jp-tosca
left a comment
There was a problem hiding this comment.
Made some comments, we also discussed that in the future we probably should create a mechanism for when a component makes an operation that may generate a notification it should trigger a refresh to check the most recent number of notifications, otherwise the user won't see the notifications until next update, not a big deal but probably would improve the user experience.
| "userGuideLinkText": "User Guide", | ||
| "demoServerLinkText": "Demo Site", | ||
| "clearAll": "Clear All", | ||
| "clearAllOnThisPage": "Clear Notifications", |
There was a problem hiding this comment.
I know this may make sense in the context it is used but do you think it would be good to have a key: "description" that is closer in meaning? "Clear notifications" is specific, while "clearAllOnThisPage" is very vague. 🤔
| void (async () => { | ||
| await markAsRead(unreadIds) | ||
| setReadIds((prev) => [...prev, ...unreadIds]) | ||
| console.log('calling refetch after marking as read') |
| .filter((n) => !n.displayAsRead && !readIds.includes(n.id)) | ||
| .map((n) => n.id) | ||
|
|
||
| console.log('in useEffect, unreadIds:', unreadIds) |
github-project-automation
Bot
moved this from In Review 🔎
to Ready for QA ⏩
in IQSS Dataverse Project
| @@ -1,27 +1,37 @@ | |||
| // TypeScript | |||
There was a problem hiding this comment.
Could you remove the // TypeScript here? and move the import styles in the end of all imports ?
There was a problem hiding this comment.
thanks, good catch :)
|
Hi @ekraffmiller , Thanks for all the work on the notification PR. It looks great! I tested it locally with hundreds of notifications. I notice that when you clear notifications in the last page, it shows Also, some suggestions for UI
|
|
thanks for the QA @ChengShi-1! I will assign it to myself to make the fixes |
|
Hi @ChengShi-1 , I made the fixes to the paging and date format, but I'm not seeing the spacing issue in the notifications count...here is what it looks like for me: 
Can you try rebuilding the design system? I think you might be missing updates in the Badge component. |
|
@ekraffmiller Thanks for the fixes! I see the badge after re-building design system, thanks.
|
|
@ChengShi-1 changes made, thank you |
github-project-automation
Bot
moved this from QA ✅
to Merged 🚀
in IQSS Dataverse Project
6 participants
What this PR does / why we need it:
Adds paging to Notifications UI
Which issue(s) this PR closes:
Special notes for your reviewer:
I created a separate "base branch" because 775 actually has all the commits for 775 and 881.
Suggestions on how to test this:
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes or changelog update needed for this change?:
Additional documentation: