-
Notifications
You must be signed in to change notification settings - Fork 21
PM-2133 dismissable banner #1260
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| name: AI PR Reviewer Updated | ||
|
|
||
| on: | ||
| pull_request: | ||
| types: | ||
| - opened | ||
| - synchronize | ||
| permissions: | ||
| pull-requests: write | ||
| jobs: | ||
| tc-ai-pr-review: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout Repo | ||
| uses: actions/checkout@v3 | ||
|
|
||
| - 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment about the |
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import { FC } from 'react' | ||
| import { toast, ToastContainer } from 'react-toastify' | ||
|
|
||
| import { useViewportUnitsFix } from '~/libs/shared' | ||
| import { useViewportUnitsFix, NotificationsContainer } from '~/libs/shared' | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
|
||
| import { AppFooter } from './components/app-footer' | ||
| import { AppHeader } from './components/app-header' | ||
|
|
@@ -14,6 +14,7 @@ const PlatformApp: FC<{}> = () => { | |
| return ( | ||
| <Providers> | ||
| <AppHeader /> | ||
| <NotificationsContainer /> | ||
| <div className='root-container'> | ||
| <PlatformRouter /> | ||
| </div> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import { FC, ReactNode } from 'react' | ||
|
|
||
| import { authUrlLogout, ProfileProvider } from '~/libs/core' | ||
| import { ConfigContextProvider } from '~/libs/shared' | ||
| import { ConfigContextProvider, NotificationProvider } from '~/libs/shared' | ||
|
|
||
| import { PlatformRouterProvider } from './platform-router.provider' | ||
|
|
||
|
|
@@ -13,7 +13,9 @@ const Providers: FC<ProvidersProps> = props => ( | |
| <ConfigContextProvider logoutUrl={authUrlLogout}> | ||
| <ProfileProvider> | ||
| <PlatformRouterProvider> | ||
| {props.children} | ||
| <NotificationProvider> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ |
||
| {props.children} | ||
| </NotificationProvider> | ||
| </PlatformRouterProvider> | ||
| </ProfileProvider> | ||
| </ConfigContextProvider> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -60,6 +60,7 @@ import { | |
| } from '../../../config/routes.config' | ||
|
|
||
| import styles from './ChallengeDetailsPage.module.scss' | ||
| import { useNotification } from '~/libs/shared' | ||
|
|
||
| interface Props { | ||
| className?: string | ||
|
|
@@ -226,6 +227,7 @@ const computePhaseCompletionFromScreenings = ( | |
|
|
||
| // eslint-disable-next-line complexity | ||
| export const ChallengeDetailsPage: FC<Props> = (props: Props) => { | ||
| const { showBannerNotification, removeNotification } = useNotification(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ |
||
| const [searchParams, setSearchParams] = useSearchParams() | ||
| const location = useLocation() | ||
| const navigate = useNavigate() | ||
|
|
@@ -1323,6 +1325,14 @@ export const ChallengeDetailsPage: FC<Props> = (props: Props) => { | |
| : undefined | ||
| const shouldShowChallengeMetaRow = Boolean(statusLabel) || trackTypePills.length > 0 | ||
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [❗❗ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The message text contains a grammatical error. It should be: 'AI Review Scores do not influence winner selection.' |
||
| }) | ||
| return () => notification && removeNotification(notification.id); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding |
||
| }, [showBannerNotification]); | ||
|
|
||
| return ( | ||
| <PageWrapper | ||
| pageTitle={challengeInfo?.name ?? ''} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,3 @@ | ||
| export * from './profile-context-data.model' | ||
| export { default as profileContext, defaultProfileContextData } from './profile.context' | ||
| export { default as profileContext, defaultProfileContextData, useProfileContext } from './profile.context' | ||
| export * from './profile.context-provider' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import { Context, createContext } from 'react' | ||
| import { Context, createContext, useContext } from 'react' | ||
|
|
||
| import { ProfileContextData } from './profile-context-data.model' | ||
|
|
||
|
|
@@ -12,4 +12,6 @@ export const defaultProfileContextData: ProfileContextData = { | |
|
|
||
| const profileContext: Context<ProfileContextData> = createContext(defaultProfileContextData) | ||
|
|
||
| export const useProfileContext = () => useContext(profileContext); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a return type to the |
||
|
|
||
| export default profileContext | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import { FC } from 'react' | ||
|
|
||
| import { Notification } from '~/libs/ui' | ||
|
|
||
| import { useNotification } from './Notifications.context'; | ||
|
|
||
| const NotificationsContainer: FC = () => { | ||
| const { notifications, removeNotification } = useNotification(); | ||
|
|
||
| return ( | ||
| <div> | ||
| {notifications.map(n => ( | ||
| <Notification key={n.id} notification={n} onClose={removeNotification} /> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider renaming the parameter There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| ))} | ||
| </div> | ||
| ) | ||
| } | ||
|
|
||
| export default NotificationsContainer | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| import React, { createContext, useContext, useState, useCallback, ReactNode } from "react"; | ||
| import { useProfileContext } from "~/libs/core"; | ||
| import { dismiss, wasDismissed } from "./localstorage.utils"; | ||
|
|
||
| export type NotificationType = "success" | "error" | "info" | "warning" | "banner"; | ||
|
|
||
| export interface Notification { | ||
| id: string; | ||
| type: NotificationType; | ||
| message: string; | ||
| duration?: number; // in ms | ||
| } | ||
|
|
||
| type NotifyPayload = string | (Partial<Notification> & { message: string }) | ||
|
|
||
| interface NotificationContextType { | ||
| notifications: Notification[]; | ||
| notify: (message: NotifyPayload, type?: NotificationType, duration?: number) => Notification | void; | ||
| showBannerNotification: (message: NotifyPayload) => Notification | void; | ||
| removeNotification: (id: string) => void; | ||
| } | ||
|
|
||
| const NotificationContext = createContext<NotificationContextType | undefined>(undefined); | ||
|
|
||
| export const useNotification = (): NotificationContextType => { | ||
| const context = useContext(NotificationContext); | ||
| if (!context) throw new Error("useNotification must be used within a NotificationProvider"); | ||
| return context; | ||
| }; | ||
|
|
||
| export const NotificationProvider: React.FC<{ | ||
| children: ReactNode, | ||
| }> = ({ children }) => { | ||
| const profileCtx = useProfileContext() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a semicolon at the end of the line for consistency with other lines. |
||
| const uuid = profileCtx.profile?.userId ?? 'annon'; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| const [notifications, setNotifications] = useState<Notification[]>([]); | ||
|
|
||
| const removeNotification = useCallback((id: string, persist?: boolean) => { | ||
| setNotifications(prev => prev.filter(n => n.id !== id)); | ||
| if (persist) { | ||
| dismiss(id); | ||
| } | ||
| }, []); | ||
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ |
||
| const newNotification: Notification = typeof message === 'string' ? { id, message, type, duration } : { type, duration, ...message, id }; | ||
|
|
||
| if (wasDismissed(id)) { | ||
| return; | ||
| } | ||
|
|
||
| setNotifications(prev => [...prev, newNotification]); | ||
|
|
||
| if (duration > 0) { | ||
| setTimeout(() => removeNotification(id), duration); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ |
||
| } | ||
|
|
||
| return newNotification; | ||
| }, | ||
| [uuid] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The dependency array for the 'notify' useCallback is missing 'removeNotification'. Ensure that all dependencies are included to prevent potential issues with stale closures. |
||
| ); | ||
|
|
||
| const showBannerNotification = useCallback(( | ||
| message: NotifyPayload, | ||
| ) => notify(message, 'banner', 0), [notify]); | ||
|
|
||
| return ( | ||
| <NotificationContext.Provider value={{ notifications, notify, showBannerNotification, removeNotification }}> | ||
| {children} | ||
| </NotificationContext.Provider> | ||
| ); | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| export { default as NotificationsContainer } from './Notifications.container' | ||
| export * from './Notifications.context' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| export const wasDismissed = (id: string): boolean => ( | ||
| (localStorage.getItem(`dismissed[${id}]`)) !== null | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [❗❗ |
||
| ) | ||
|
|
||
| export const dismiss = (id: string): void => { | ||
| localStorage.setItem(`dismissed[${id}]`, JSON.stringify(true)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ |
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| import { FC } from 'react' | ||
|
|
||
| import { NotificationBanner } from './banner' | ||
|
|
||
| interface NotificationProps { | ||
| notification: { message: string; id: string; type: string } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using a more specific type for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ |
||
| onClose: (id: string, save?: boolean) => void | ||
| } | ||
|
|
||
| const Notification: FC<NotificationProps> = props => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [💡 |
||
|
|
||
| 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ |
||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| export default Notification | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| @import '../../../styles/includes'; | ||
|
|
||
| .wrap { | ||
| background: #60267D; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using a more descriptive variable name for the background color instead of hardcoding the hex value |
||
| color: $tc-white; | ||
|
|
||
| .inner { | ||
| max-width: $xxl-min; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The variable |
||
| padding: $sp-3 0; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Padding values |
||
| @include pagePaddings; | ||
| margin: 0 auto; | ||
| width: 100%; | ||
| display: flex; | ||
| justify-content: space-between; | ||
| align-items: center; | ||
| @include ltemd { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The mixin There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ |
||
| display: block; | ||
| position: relative; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| .close { | ||
| cursor: pointer; | ||
| color: $tc-white; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| /* eslint-disable no-underscore-dangle */ | ||
| /* eslint-disable camelcase */ | ||
|
|
||
| import { Meta, StoryObj } from '@storybook/react' | ||
|
|
||
| import NotificationBanner from './NotificationBanner' | ||
|
|
||
| const meta: Meta<typeof NotificationBanner> = { | ||
| argTypes: { | ||
| persistent: { | ||
| defaultValue: false, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The description for the |
||
| description: 'Set to true to allow clicks inside the tooltip', | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ |
||
| }, | ||
| content: { | ||
| description: 'Content displayed inside the tooltip', | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The description for the |
||
| }, | ||
| }, | ||
| component: NotificationBanner, | ||
| excludeStories: /.*Decorator$/, | ||
| tags: ['autodocs'], | ||
| title: 'Components/NotificationBanner', | ||
| } | ||
|
|
||
| export default meta | ||
|
|
||
| type Story = StoryObj<typeof NotificationBanner>; | ||
|
|
||
| export const Primary: Story = { | ||
| args: { | ||
| // children: <IconOutline.QuestionMarkCircleIcon width='35' />, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The commented-out line for |
||
| content: 'Help tooltip', | ||
| }, | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import { FC, ReactNode, useCallback } from 'react' | ||
|
|
||
| import styles from './NotificationBanner.module.scss' | ||
| import { InformationCircleIcon, XCircleIcon } from '@heroicons/react/outline' | ||
|
|
||
| interface NotificationBannerProps { | ||
| persistent?: boolean | ||
| content: ReactNode | ||
| icon?: ReactNode | ||
| onClose?: (save?: boolean) => void | ||
| } | ||
|
|
||
| const NotificationBanner: FC<NotificationBannerProps> = props => { | ||
|
|
||
| return ( | ||
| <div className={styles.wrap}> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a more descriptive class name to |
||
| <div className={styles.inner}> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider adding a more descriptive class name to |
||
| {props.icon || ( | ||
| <div> | ||
| <InformationCircleIcon className='icon-xl' /> | ||
| </div> | ||
| )} | ||
|
|
||
| {props.content} | ||
|
|
||
| {!props.persistent && ( | ||
| <div className={styles.close} onClick={() => props.onClose?.(true)}> | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| <XCircleIcon className='icon-xl' /> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ) | ||
| } | ||
|
|
||
| export default NotificationBanner | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| export { default as NotificationBanner } from './NotificationBanner' |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| export * from './banner' | ||
| export { default as Notification } from './Notification' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[💡
readability]The comment regarding the
GITHUB_TOKENis 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.