PM-2133 dismissable banner #1261
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
...ps/review/src/pages/active-review-assignements/ChallengeDetailsPage/ChallengeDetailsPage.tsx
Show resolved
Hide resolved
...ps/review/src/pages/active-review-assignements/ChallengeDetailsPage/ChallengeDetailsPage.tsx
Show resolved
Hide resolved
| id: 'ai-review-scores-warning', | ||
| message: `AI Review Scores are advisory only to provide immediate, | ||
| educational, and actionable feedback to members. | ||
| AI Review Scores are not influence winner selection.`, |
There was a problem hiding this comment.
The message text contains a grammatical error. It should be 'AI Review Scores do not influence winner selection.' instead of 'AI Review Scores are not influence winner selection.'
| id: 'ai-review-scores-warning', | ||
| message: `AI Review Scores are advisory only to provide immediate, | ||
| educational, and actionable feedback to members. | ||
| AI Review Scores are not influence winner selection.`, |
There was a problem hiding this comment.
[❗❗ correctness]
Typo in the message: 'AI Review Scores are not influence winner selection.' should be 'AI Review Scores do not influence winner selection.'
...ps/review/src/pages/active-review-assignements/ChallengeDetailsPage/ChallengeDetailsPage.tsx
Show resolved
Hide resolved
src/libs/shared/lib/components/notifications/Notifications.container.tsx
Show resolved
Hide resolved
src/libs/shared/lib/components/notifications/Notifications.container.tsx
Show resolved
Hide resolved
src/libs/shared/lib/components/notifications/Notifications.container.tsx
Show resolved
Hide resolved
| const Notification: FC<NotificationProps> = props => { | ||
| const handleClose = useCallback((save?: boolean) => { | ||
| props.onClose(props.notification.id, save) | ||
| }, [props.onClose]) |
There was a problem hiding this comment.
The dependency array for useCallback should include props.notification.id to ensure that the callback is updated if the id changes.
src/libs/ui/lib/components/notification/banner/NotificationBanner.module.scss
Show resolved
Hide resolved
src/libs/ui/lib/components/notification/banner/NotificationBanner.module.scss
Show resolved
Hide resolved
src/libs/shared/lib/components/notifications/localstorage.utils.ts
Outdated
Show resolved
Hide resolved
src/libs/shared/lib/components/notifications/localstorage.utils.ts
Outdated
Show resolved
Hide resolved
src/libs/ui/lib/components/notification/banner/NotificationBanner.stories.tsx
Outdated
Show resolved
Hide resolved
src/libs/ui/lib/components/notification/banner/NotificationBanner.stories.tsx
Outdated
Show resolved
Hide resolved
src/libs/ui/lib/components/notification/banner/NotificationBanner.module.scss
Show resolved
Hide resolved
src/libs/ui/lib/components/notification/banner/NotificationBanner.module.scss
Show resolved
Hide resolved
src/libs/ui/lib/components/notification/banner/NotificationBanner.stories.tsx
Show resolved
Hide resolved
src/libs/ui/lib/components/notification/banner/NotificationBanner.stories.tsx
Show resolved
Hide resolved
| - opened | ||
| - synchronize | ||
| permissions: | ||
| pull-requests: write |
There was a problem hiding this comment.
[security]
Consider specifying more granular permissions instead of pull-requests: write to follow the principle of least privilege. This can help minimize potential security risks.
| uses: topcoder-platform/tc-ai-pr-reviewer@prompt-v2 | ||
| with: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} # The GITHUB_TOKEN is there by default so you just need to keep it like it is and not necessarily need to add it as secret as it will throw an error. [More Details](https://docs.github.com/en/actions/security-guides/automatic-token-authentication#about-the-github_token-secret) | ||
| LAB45_API_KEY: ${{ secrets.LAB45_API_KEY }} |
There was a problem hiding this comment.
[💡 maintainability]
The exclude pattern uses a comma-separated list, which is correct, but ensure that the patterns are comprehensive enough to cover all file types you intend to exclude. Consider adding comments to clarify the purpose of excluding these specific file types.
| const canViewChallenge = isAdmin || isUserResource | ||
| const statusLabel = isPastReviewDetail | ||
| ? formatChallengeStatusLabel(challengeInfo?.status) | ||
| : undefined |
There was a problem hiding this comment.
[correctness]
The useEffect hook that displays the banner notification does not include removeNotification in its dependency array. This could lead to stale closures if removeNotification changes. Consider adding removeNotification to the dependency array.
| const shouldShowChallengeMetaRow = Boolean(statusLabel) || trackTypePills.length > 0 | ||
|
|
||
| useEffect(() => { | ||
| const notification = showBannerNotification({ |
There was a problem hiding this comment.
[❗❗ correctness]
The message in showBannerNotification contains a typo: 'AI Review Scores are not influence winner selection.' should be 'AI Review Scores do not influence winner selection.'
| const { notifications, removeNotification }: NotificationContextType = useNotification() | ||
|
|
||
| return ( | ||
| <div className={styles.wrap}> |
There was a problem hiding this comment.
[maintainability]
Consider adding a type for the notifications array elements to ensure type safety and improve maintainability. This will help catch errors if the structure of notification objects changes in the future.
| const [notifications, setNotifications] = useState<Notification[]>([]) | ||
|
|
||
| const removeNotification = useCallback((id: string, persist?: boolean) => { | ||
| setNotifications(prev => prev.filter(n => n.id !== id)) |
There was a problem hiding this comment.
[correctness]
The id generation logic using uuid and message.id assumes that message.id is unique and defined when message is an object. Consider adding a check or fallback for cases where message.id might be undefined to avoid potential id collisions.
| }, []) | ||
|
|
||
| const notify = useCallback( | ||
| (message: NotifyPayload, type: NotificationType = 'info', duration = 3000) => { |
There was a problem hiding this comment.
[performance]
The setTimeout function used for removing notifications after a certain duration does not clear the timeout if the component unmounts before the timeout completes. This could lead to memory leaks. Consider storing the timeout ID and clearing it in a cleanup function.
| @@ -0,0 +1,9 @@ | |||
| const lsKeyPrefix = 'notificationDismissed' | |||
There was a problem hiding this comment.
[maintainability]
Consider using a more descriptive prefix for lsKeyPrefix to avoid potential key collisions in localStorage. This will improve maintainability and reduce the risk of overwriting unrelated data.
| const lsKeyPrefix = 'notificationDismissed' | ||
|
|
||
| export const wasDismissed = (id: string): boolean => ( | ||
| (localStorage.getItem(`${lsKeyPrefix}[${id}]`)) !== null |
There was a problem hiding this comment.
[design]
The use of localStorage directly ties this utility to the browser environment. If this utility is intended to be used in environments other than the browser, consider abstracting the storage mechanism to improve portability.
| interface NotificationProps { | ||
| notification: { | ||
| icon?: ReactNode; | ||
| id: string; |
There was a problem hiding this comment.
[correctness]
Consider using a union type for notification.type to restrict it to known values like 'banner'. This will help catch errors at compile time if an unsupported type is used.
| onClose={handleClose} | ||
| /> | ||
| ) | ||
| } |
There was a problem hiding this comment.
[maintainability]
Returning an empty fragment (<></>) when the notification type is not 'banner' might not be the best approach. Consider handling other types explicitly or logging a warning for unsupported types to aid debugging.
| @import '../../../styles/includes'; | ||
|
|
||
| .wrap { | ||
| background: #60267D; |
There was a problem hiding this comment.
[maintainability]
Consider using a CSS variable or a SCSS variable for the background color #60267D to improve maintainability and consistency across the project.
| color: $tc-white; | ||
| flex: 0 0; | ||
| margin-left: auto; | ||
| border-radius: 50%; |
There was a problem hiding this comment.
[maintainability]
The use of border: 2px solid white could be replaced with $tc-white to ensure consistency with the color scheme defined in your SCSS variables.
| description: 'Content displayed inside the notification banner', | ||
| }, | ||
| persistent: { | ||
| defaultValue: false, |
There was a problem hiding this comment.
[maintainability]
Consider adding a default value for the content prop to ensure the component behaves predictably even if no content is provided. This can improve maintainability and prevent potential runtime errors.
| } | ||
|
|
||
| const NotificationBanner: FC<NotificationBannerProps> = props => { | ||
| const handleClose = useCallback(() => { |
There was a problem hiding this comment.
[💡 performance]
Using useCallback here is unnecessary unless handleClose is passed to a child component that relies on referential equality for optimization. Consider removing it to simplify the code.
| )} | ||
|
|
||
| {props.content} | ||
|
|
There was a problem hiding this comment.
[design]
Using onClick on a div for the close button may not be accessible for keyboard users. Consider using a button element instead to improve accessibility.
|
closing as there's too much noise in the comments |
Related JIRA Ticket:
https://topcoder.atlassian.net/browse/PM-2133 - dismissable banner
What's in this PR?
Created component for notification banner that's also dismisable. Also created context for notifications so we can easily use them across platform-ui. This is heavily extendable. I only created the basics, but it can easily be extended to other type of notifications/banners.