[ENH] Implement Notification Icon with Badge for Warnings and Its Test #432
Conversation
Reviewer's Guide by SourceryThis pull request implements a notification icon with a badge in the navigation bar to display non-critical warnings to the user. It also includes a test for the new notification badge feature. Sequence diagram for the new warning notification flowsequenceDiagram
participant User
participant App
participant Navbar
participant NotificationSystem
App->>NotificationSystem: Initialize warnings state
App->>Navbar: Render with notifications prop
Note over Navbar: Display notification badge
User->>Navbar: Click notification icon
Navbar->>NotificationSystem: Open notification popover
NotificationSystem-->>User: Display warning messages
User->>NotificationSystem: View warnings
User->>NotificationSystem: Close notification popover
Class diagram for updated Navbar componentclassDiagram
class Navbar {
+boolean isLoggedIn
+function onLogin()
+string[] notifications
-HTMLElement anchorEl
-HTMLElement anchorMailEl
-boolean openAccountMenu
-boolean openMailMenu
+handleClick(event)
+handleClose()
+handleMailClick(event)
+handleMailClose()
}
note for Navbar "Added notifications support"
class Badge {
+number badgeContent
+string color
}
class NotificationsIcon {
+string color
}
Navbar --> Badge
Navbar --> NotificationsIcon
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
bot
added
the
_community
✅ Deploy Preview for neurobagel-query ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
rmanaem
added
the
pr-patch
There was a problem hiding this comment.
Hi @Tusharjamdade,
Congratulations on your first contribution to Neurobagel! 🎉 The changes look great overall. However, there are a few areas that need adjustments:
- Currently, the implementation doesn't support deleting notifications, either individually or all at once.
- The way warnings are passed to the Navbar component is too specific, limiting reusability.
- Some of the e2e and component tests are failing due to these changes.
To address these, we can:
- Implement delete functionality for each notification and a "clear all notifications" feature for the popover.
- Refactor the logic for passing notifications to the Navbar component to make it more generic. This will allow us to pass different types of notifications (info, errors) to the Navbar.
- For that the type of notification so that the appropriate title and color can be set in the ListItemText.
- Once the logic is updated, we can also display info notifications in the popover.
- Update the tests to reflect these changes.
I understand this might be overwhelming for your first PR and contribution. Please don't hesitate to reach out with any questions, or let us know if you would prefer one of the maintainers to take over this PR.
| const [anchorEl, setAnchorEl] = useState<null | HTMLElement>(null); | ||
| const openAccountMenu = Boolean(anchorEl); | ||
|
|
||
| const { user, logout } = useAuth0(); | ||
| const [anchorMailEl, setAnchorMailEl] = useState<null | HTMLElement>(null); |
There was a problem hiding this comment.
| const [anchorMailEl, setAnchorMailEl] = useState<null | HTMLElement>(null); | |
| const [anchorNotifEl, setAnchorNotifEl] = useState<null | HTMLElement>(null); |
Since we switched out the mail icon for notification, it's good to update the relevant variables.
| const [anchorEl, setAnchorEl] = useState<null | HTMLElement>(null); | ||
| const openAccountMenu = Boolean(anchorEl); | ||
|
|
||
| const { user, logout } = useAuth0(); | ||
| const [anchorMailEl, setAnchorMailEl] = useState<null | HTMLElement>(null); | ||
| const openMailMenu = Boolean(anchorMailEl); |
There was a problem hiding this comment.
| const openMailMenu = Boolean(anchorMailEl); | |
| const openNotifMenu = Boolean(anchorNotifEl); |
| const handleMailClick = (event: React.MouseEvent<HTMLElement>) => { | ||
| setAnchorMailEl(event.currentTarget); | ||
| }; |
There was a problem hiding this comment.
| const handleMailClick = (event: React.MouseEvent<HTMLElement>) => { | |
| setAnchorMailEl(event.currentTarget); | |
| }; | |
| const handleNotifClick = (event: React.MouseEvent<HTMLElement>) => { | |
| setAnchorNotifEl(event.currentTarget); | |
| }; |
| const handleMailClose = () => { | ||
| setAnchorMailEl(null); |
There was a problem hiding this comment.
| const handleMailClose = () => { | |
| setAnchorMailEl(null); | |
| const handleNotifClose = () => { | |
| setAnchorNotifEl(null); |
| @@ -56,9 +81,83 @@ function Navbar({ isLoggedIn, onLogin }: { isLoggedIn: boolean; onLogin: () => v | |||
| <Article /> | |||
| </IconButton> | |||
| </Tooltip> | |||
| <Tooltip title="Notifications"> | |||
| <IconButton onClick={handleMailClick}> | |||
There was a problem hiding this comment.
| <IconButton onClick={handleMailClick}> | |
| <IconButton onClick={handleNotifClick}> |
| vertical: 'top', | ||
| horizontal: 'right', | ||
| }} | ||
| PaperProps={{ |
There was a problem hiding this comment.
I believe the PaperProps is deprecated see the relevant documentation section here
| primary={ | ||
| <Typography variant="subtitle2" className="font-bold text-[#E65100]"> | ||
| {/* Add the appropriate title for the notification if any */} | ||
| </Typography> |
There was a problem hiding this comment.
I think we can use the type of notification e.g., warning, error, info for the primary content.
| <IconButton | ||
| edge="end" | ||
| aria-label="delete" | ||
| className="text-[#FF5722] hover:bg-[#FF572220]" | ||
| /> |
There was a problem hiding this comment.
You may have forgotten to add the delete icon and implement the logic for deleting a list item.
| <ListItem | ||
| key={notification} | ||
| alignItems="flex-start" | ||
| className="mb-2 rounded-md border-l-4 border-[#FF9800] bg-[#FFF3E0] transition-colors duration-200 hover:bg-gray-200" |
There was a problem hiding this comment.
The issue with this implementation is it doesn't take into account the type of notification and colors them all the same see other comments for more context
| @@ -24,5 +31,7 @@ describe('Navbar', () => { | |||
| cy.get("[data-cy='navbar'] a") | |||
| .eq(1) | |||
| .should('have.attr', 'href', 'https://github.com/neurobagel/query-tool/'); | |||
| // Test for Notification Icon and Badge | |||
| cy.get("[data-cy='navbar'] button").eq(1).find('svg').should('be.visible'); // Check if the notification icon exists | |||
There was a problem hiding this comment.
It's good to expand this test to include some assertions about the contents of the popover
Thank you for the review, @rmanaem. I appreciate the detailed feedback and suggestions. |
Hi @rmanaem export type Notification = {
id: number;
type: 'info' | 'warning';
message: string;
};
const [notifications, setNotifications] = useState<Notification[]>([]);
<Navbar
isLoggedIn={isAuthenticated}
onLogin={() => setOpenAuthDialog(true)}
notifications={notifications}
setNotifications={setNotifications}
/>
// Navbar component
function Navbar({
isLoggedIn,
onLogin,
notifications,
setNotifications
}: {
isLoggedIn: boolean;
onLogin: () => void;
notifications: Notification[];
setNotifications: React.Dispatch<React.SetStateAction<Notification[]>>;
}) This is an overview of the implementation I have used. I have stored all notifications based on their types (warning, info) and messages, assigning each one a unique identifier (which will be used to delete the notification when the delete icon is clicked). I passed the notifications and setNotifications to update the notifications (when the user clicks the delete button or the clear all button). Is this approach acceptable, or do I need to work on something else? |
|
Hi @Tusharjamdade, |
Changes proposed in this pull request:
Checklist
This section is for the PR reviewer
[ENH],[FIX],[REF],[TST],[CI],[MNT],[INF],[MODEL],[DOC]) (see our Contributing Guidelines for more info)skip-release(to be applied by maintainers only)Closes #XXXXquery-tool-resultsfiles in the neurobagel_examples repo have been regeneratedFor new features:
For bug fixes:
Summary by Sourcery
Add a notification icon with a badge to display non-critical warnings to the user.
New Features:
Tests: