PM-2133 dismissable banner #1260
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| - name: TC AI PR Reviewer | ||
| uses: topcoder-platform/tc-ai-pr-reviewer@prompt-update | ||
| 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) |
There was a problem hiding this comment.
[💡 readability]
The comment regarding the GITHUB_TOKEN is informative but could be misleading. It's important to ensure that users understand that the token is automatically provided by GitHub Actions and does not need to be manually added to the repository secrets. Consider clarifying this to prevent potential confusion.
| - name: TC AI PR Reviewer | ||
| uses: topcoder-platform/tc-ai-pr-reviewer@prompt-update | ||
| 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) |
There was a problem hiding this comment.
The comment about the GITHUB_TOKEN is informative but might be better suited for documentation rather than inline in the workflow file. Consider removing or relocating it to a more appropriate place.
| 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 }} | ||
| exclude: '**/*.json, **/*.md, **/*.jpg, **/*.png, **/*.jpeg, **/*.bmp, **/*.webp' # Optional: exclude patterns separated by commas |
There was a problem hiding this comment.
The exclude pattern is specified as a string with comma-separated values. Ensure that this format is supported by the action being used, as some actions might expect an array format instead.
| import { toast, ToastContainer } from 'react-toastify' | ||
|
|
||
| import { useViewportUnitsFix } from '~/libs/shared' | ||
| import { useViewportUnitsFix, NotificationsContainer } from '~/libs/shared' |
There was a problem hiding this comment.
The NotificationsContainer import is added here, but it is not used in this code snippet. Ensure that it is utilized appropriately in the component or remove the import if it is unnecessary.
| <ProfileProvider> | ||
| <PlatformRouterProvider> | ||
| {props.children} | ||
| <NotificationProvider> |
There was a problem hiding this comment.
[correctness]
Consider verifying that NotificationProvider correctly handles the lifecycle of its children components, especially if it introduces asynchronous operations or side effects. This ensures that the integration does not inadvertently affect the rendering or behavior of props.children.
|
|
||
| // eslint-disable-next-line complexity | ||
| export const ChallengeDetailsPage: FC<Props> = (props: Props) => { | ||
| const { showBannerNotification, removeNotification } = useNotification(); |
There was a problem hiding this comment.
[maintainability]
Ensure that useNotification is correctly imported and that showBannerNotification and removeNotification are used appropriately within the component. If these functions are not used, consider removing them to avoid unnecessary imports and potential confusion.
| useEffect(() => { | ||
| const notification = showBannerNotification({ | ||
| 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]
The message string contains a grammatical error: 'AI Review Scores are not influence winner selection.' should be 'AI Review Scores do 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.', | ||
| }) | ||
| return () => notification && removeNotification(notification.id); |
There was a problem hiding this comment.
[correctness]
Ensure that removeNotification is a safe operation even if notification is undefined. Consider adding a null check before calling removeNotification(notification.id).
| useEffect(() => { | ||
| const notification = showBannerNotification({ | ||
| 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.'
| 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.', | ||
| }) | ||
| return () => notification && removeNotification(notification.id); |
There was a problem hiding this comment.
Consider adding removeNotification to the dependency array of the useEffect hook to ensure it is always the latest version when the effect runs.
|
|
||
| const profileContext: Context<ProfileContextData> = createContext(defaultProfileContextData) | ||
|
|
||
| export const useProfileContext = () => useContext(profileContext); |
There was a problem hiding this comment.
Consider adding a return type to the useProfileContext function for better type safety and clarity.
| return ( | ||
| <div> | ||
| {notifications.map(n => ( | ||
| <Notification key={n.id} notification={n} onClose={removeNotification} /> |
There was a problem hiding this comment.
[correctness]
Passing removeNotification directly as the onClose handler may lead to incorrect behavior if removeNotification requires any arguments or context. Consider wrapping it in a function to ensure it is called with the correct parameters.
| return ( | ||
| <div> | ||
| {notifications.map(n => ( | ||
| <Notification key={n.id} notification={n} onClose={removeNotification} /> |
There was a problem hiding this comment.
Consider renaming the parameter n to something more descriptive, like notification, to improve code readability.
| return ( | ||
| <div> | ||
| {notifications.map(n => ( | ||
| <Notification key={n.id} notification={n} onClose={removeNotification} /> |
There was a problem hiding this comment.
The onClose prop is being passed the removeNotification function directly. Ensure that removeNotification is correctly handling the notification ID or object to remove the specific notification, as it might need an argument to identify which notification to remove.
| export const NotificationProvider: React.FC<{ | ||
| children: ReactNode, | ||
| }> = ({ children }) => { | ||
| const profileCtx = useProfileContext() |
There was a problem hiding this comment.
[correctness]
Consider handling the case where profileCtx.profile is undefined more explicitly. Currently, it defaults to 'annon', which might lead to unexpected behavior if the user ID is crucial for notification identification.
|
|
||
| const notify = useCallback( | ||
| (message: NotifyPayload, type: NotificationType = "info", duration = 3000) => { | ||
| const id = `${uuid}[${typeof message === 'string' ? message : message.id}]`; |
There was a problem hiding this comment.
[correctness]
The ID generation logic concatenates uuid with the message ID or message string. Ensure that the message string does not contain characters that could lead to unexpected results in the ID format, such as brackets.
| setNotifications(prev => [...prev, newNotification]); | ||
|
|
||
| if (duration > 0) { | ||
| setTimeout(() => removeNotification(id), duration); |
There was a problem hiding this comment.
[performance]
The setTimeout function does not clear itself if the component unmounts before the timeout completes. Consider using a cleanup function to clear the timeout to prevent potential memory leaks.
| export const NotificationProvider: React.FC<{ | ||
| children: ReactNode, | ||
| }> = ({ children }) => { | ||
| const profileCtx = useProfileContext() |
There was a problem hiding this comment.
Consider adding a semicolon at the end of the line for consistency with other lines.
| children: ReactNode, | ||
| }> = ({ children }) => { | ||
| const profileCtx = useProfileContext() | ||
| const uuid = profileCtx.profile?.userId ?? 'annon'; |
There was a problem hiding this comment.
The string 'annon' seems to be a placeholder for anonymous users. Consider using a more descriptive variable name or adding a comment in the code to clarify its purpose.
|
|
||
| return newNotification; | ||
| }, | ||
| [uuid] |
There was a problem hiding this comment.
The dependency array for the 'notify' useCallback is missing 'removeNotification'. Ensure that all dependencies are included to prevent potential issues with stale closures.
| @@ -0,0 +1,7 @@ | |||
| export const wasDismissed = (id: string): boolean => ( | |||
| (localStorage.getItem(`dismissed[${id}]`)) !== null | |||
There was a problem hiding this comment.
[❗❗ correctness]
Using localStorage directly can lead to issues in environments where it is not available (e.g., server-side rendering or private browsing modes). Consider adding a check to ensure localStorage is available before accessing it.
| ) | ||
|
|
||
| export const dismiss = (id: string): void => { | ||
| localStorage.setItem(`dismissed[${id}]`, JSON.stringify(true)) |
There was a problem hiding this comment.
[maintainability]
Consider using a consistent prefix or namespace for localStorage keys to avoid potential key collisions with other parts of the application or third-party scripts.
| import { NotificationBanner } from './banner' | ||
|
|
||
| interface NotificationProps { | ||
| notification: { message: string; id: string; type: string } |
There was a problem hiding this comment.
Consider using a more specific type for the type property in the notification object, such as a union type (e.g., 'banner' | 'alert' | 'info') to restrict the possible values and improve type safety.
| const Notification: FC<NotificationProps> = props => { | ||
|
|
||
| if (props.notification.type === 'banner') { | ||
| return <NotificationBanner content={props.notification.message} onClose={(save?: boolean) => props.onClose(props.notification.id, save)} /> |
There was a problem hiding this comment.
The onClose function is being passed a parameter save which is optional. Ensure that the NotificationBanner component correctly handles this optional parameter, or consider providing a default value if necessary.
| import { NotificationBanner } from './banner' | ||
|
|
||
| interface NotificationProps { | ||
| notification: { message: string; id: string; type: string } |
There was a problem hiding this comment.
[maintainability]
Consider using a TypeScript enum for type instead of a string to improve type safety and maintainability.
| onClose: (id: string, save?: boolean) => void | ||
| } | ||
|
|
||
| const Notification: FC<NotificationProps> = props => { |
There was a problem hiding this comment.
[💡 readability]
Destructuring props in the function parameter list could improve readability and reduce repetitive access to props.
| const Notification: FC<NotificationProps> = props => { | ||
|
|
||
| if (props.notification.type === 'banner') { | ||
| return <NotificationBanner content={props.notification.message} onClose={(save?: boolean) => props.onClose(props.notification.id, save)} /> |
There was a problem hiding this comment.
[correctness]
Consider handling other notification types or explicitly documenting that only 'banner' type is supported to prevent potential misuse.
| @import '../../../styles/includes'; | ||
|
|
||
| .wrap { | ||
| background: #60267D; |
There was a problem hiding this comment.
Consider using a more descriptive variable name for the background color instead of hardcoding the hex value #60267D. This will improve maintainability and readability.
| color: $tc-white; | ||
|
|
||
| .inner { | ||
| max-width: $xxl-min; |
There was a problem hiding this comment.
The variable $xxl-min is used for max-width. Ensure this variable is defined and correctly represents the intended size for the banner.
|
|
||
| .inner { | ||
| max-width: $xxl-min; | ||
| padding: $sp-3 0; |
There was a problem hiding this comment.
Padding values $sp-3 0 are used. Verify that $sp-3 is defined and represents the desired spacing for the banner.
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| @include ltemd { |
There was a problem hiding this comment.
The mixin ltemd is used here. Ensure that this mixin is defined and correctly applies the intended styles for medium screen sizes.
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| @include ltemd { |
There was a problem hiding this comment.
[maintainability]
The @include ltemd mixin usage is not clear from the context. Ensure that this mixin is defined and behaves as expected, especially since it changes the layout to display: block and position: relative, which could affect the banner's responsiveness and layout.
| const meta: Meta<typeof NotificationBanner> = { | ||
| argTypes: { | ||
| persistent: { | ||
| defaultValue: false, |
There was a problem hiding this comment.
[correctness]
The description for persistent seems incorrect. It mentions allowing clicks inside the tooltip, but this is a notification banner. Ensure the description accurately reflects the functionality of the persistent property for this component.
| argTypes: { | ||
| persistent: { | ||
| defaultValue: false, | ||
| description: 'Set to true to allow clicks inside the tooltip', |
There was a problem hiding this comment.
[correctness]
The description for persistent should be updated to accurately describe its purpose in the context of a notification banner, as it currently seems to describe tooltip behavior.
| const meta: Meta<typeof NotificationBanner> = { | ||
| argTypes: { | ||
| persistent: { | ||
| defaultValue: false, |
There was a problem hiding this comment.
The description for the persistent property mentions allowing clicks inside the tooltip, but the property name persistent might not clearly convey this functionality. Consider renaming it to something more descriptive, like allowClicksInside.
| description: 'Set to true to allow clicks inside the tooltip', | ||
| }, | ||
| content: { | ||
| description: 'Content displayed inside the tooltip', |
There was a problem hiding this comment.
The description for the content property mentions displaying content inside the tooltip, but the property name content might not clearly convey that it is specifically for tooltips. Consider renaming it to something more descriptive, like tooltipContent.
|
|
||
| export const Primary: Story = { | ||
| args: { | ||
| // children: <IconOutline.QuestionMarkCircleIcon width='35' />, |
There was a problem hiding this comment.
The commented-out line for children suggests that it might be intended for future use or testing. If this is not needed, consider removing it to keep the code clean and avoid confusion.
| {props.content} | ||
|
|
||
| {!props.persistent && ( | ||
| <div className={styles.close} onClick={() => props.onClose?.(true)}> |
There was a problem hiding this comment.
[performance]
Using an inline function for the onClick handler can lead to unnecessary re-renders of the component. Consider using useCallback to memoize the handler function.
| const NotificationBanner: FC<NotificationBannerProps> = props => { | ||
|
|
||
| return ( | ||
| <div className={styles.wrap}> |
There was a problem hiding this comment.
Consider adding a more descriptive class name to styles.wrap to better convey its purpose or role within the component.
|
|
||
| return ( | ||
| <div className={styles.wrap}> | ||
| <div className={styles.inner}> |
There was a problem hiding this comment.
Consider adding a more descriptive class name to styles.inner to better convey its purpose or role within the component.
| {props.content} | ||
|
|
||
| {!props.persistent && ( | ||
| <div className={styles.close} onClick={() => props.onClose?.(true)}> |
There was a problem hiding this comment.
The onClick handler for the close button uses an inline function. Consider defining the function separately to improve readability and potentially optimize performance by preventing unnecessary re-creations of the function on each render.
|
closing as this is not ready yet |
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.