feat(ui/portal): create dashboard #309
Conversation
✅ Deploy Preview for email-cuhacking ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
View your CI Pipeline Execution ↗ for commit c75099a.
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughThis pull request updates multiple project files to add and enhance UI components along with their Storybook configurations. New components include Tooltip (using Radix UI), Sidebar with responsive state management, Banner, HackathonCountdown with a countdown timer, Legal, Stat, UserStatus, and an animated Logo. The sign-in route has been replaced with a new implementation featuring a background image and the Home component. Several export index files have been updated to streamline module imports. Additionally, new dependencies for Radix UI Tooltip and motion animations have been added, and minor styling adjustments were made to components like Icon and TerminalText. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (1)
package.json (1)
87-87: 🛠️ Refactor suggestionConsider consolidating animation libraries.
The project already includes
framer-motion, which provides similar functionality to the newly addedmotionlibrary. Having both animation libraries could lead to:
- Increased bundle size
- Inconsistent animation patterns across components
- Additional maintenance overhead
Consider standardizing on
framer-motionas it's more feature-rich and already part of the project.Also applies to: 92-92
🧹 Nitpick comments (22)
libs/portal/ui/dashboard/stat/stat.tsx (2)
4-4: Extract props interface for better maintainability.Consider extracting the props interface for better maintainability and reusability.
+interface StatProps { + label: string; + value: string; +} + -export function Stat({ label, value }: { label: string, value: string }) { +export function Stat({ label, value }: StatProps) {
4-11: Add test coverage for the Stat component.As mentioned in the PR objectives, test coverage is missing. Please add unit tests to verify:
- Component rendering
- Props validation
- Accessibility features
Would you like me to help generate test cases for this component?
libs/portal/ui/dashboard/legal/legal.stories.tsx (1)
8-11: Document component props in Storybook configuration.The empty
argsandargTypesobjects suggest missing prop documentation. Consider adding:
- Default values in
args- Control types and descriptions in
argTypesThis will improve component documentation and testing capabilities in Storybook.
libs/portal/ui/dashboard/banner/banner.tsx (2)
3-3: Add prop types validation.Consider using TypeScript interface or type alias for props definition:
+interface BannerProps { + name: string; +} -export function Banner({ name }: { name: string }) +export function Banner({ name }: BannerProps)
3-12: Add unit tests for the Banner component.The PR objectives indicate missing tests. Consider adding tests for:
- Rendering with different name values
- Verifying welcome message
- Checking CSS classes
Would you like me to generate the test file with these test cases?
libs/portal/ui/dashboard/banner/banner.stories.tsx (1)
18-20: Add more stories to showcase different states.Consider adding stories for:
- Long names to demonstrate text wrapping
- Special characters in names
- Different viewport sizes
Example:
export const LongName: Story = { args: { name: 'Very Long Name That Might Wrap', }, }; export const SpecialCharacters: Story = { args: { name: '🎉 John Doe!', }, };apps/portal/app/routes/_index.tsx (2)
4-4: Consider renaming the component to better reflect its purpose.The component name
Signinseems inconsistent with its purpose as it appears to be a dashboard view based on the imported components and background image.Consider renaming it to
Dashboardor similar to better reflect its functionality:-export default function Signin() { +export default function Dashboard() {
8-13: LGTM! Consider optimizing the background image.The implementation looks good with proper positioning and accessibility attributes. However, consider adding loading and decoding attributes for better performance.
<img src={dashboard_background} alt="Background" aria-hidden="true" + loading="eager" + decoding="async" className="absolute top-0 left-0 w-full h-full object-cover z-[-1]" />libs/portal/ui/dashboard/hackathon-countdown/countdown.ts (1)
1-22: Simplify the calculation logic and add tests.The implementation is correct but could be simplified. Also, as mentioned in the PR objectives, tests are missing.
Consider this simpler implementation using division and modulo:
export function getCountdownTo(date: Date): { days: number, hrs: number, mins: number, secs: number } { const now = new Date() if (now > date) { throw new Error('Target date must be in the future.') } - let diff = date.getTime() - now.getTime() - - const days = Math.floor(diff / (1000 * 60 * 60 * 24)) - diff -= days * 1000 * 60 * 60 * 24 - - const hrs = Math.floor(diff / (1000 * 60 * 60)) - diff -= hrs * 1000 * 60 * 60 - - const mins = Math.floor(diff / (1000 * 60)) - diff -= mins * 1000 * 60 - - const secs = Math.floor(diff / 1000) + const diff = Math.floor((date.getTime() - now.getTime()) / 1000) + const days = Math.floor(diff / (60 * 60 * 24)) + const hrs = Math.floor((diff % (60 * 60 * 24)) / (60 * 60)) + const mins = Math.floor((diff % (60 * 60)) / 60) + const secs = diff % 60 return { days, hrs, mins, secs } }Would you like me to generate test cases for this function? The tests should cover:
- Valid future dates
- Invalid past dates
- Edge cases like same day/time
libs/shared/ui/terminal-text/terminal-text.tsx (1)
35-35: Consider using CSS variables for dimensions.The placeholder uses fixed dimensions which might not scale well with different font sizes.
Consider using CSS variables for better maintainability:
- {!icon ? <div className="text-muted text-[24px] w-[24px]">~</div> : <Icon media={icon} />} + {!icon ? ( + <div + className="text-muted" + style={{ + fontSize: 'var(--terminal-icon-size, 24px)', + width: 'var(--terminal-icon-size, 24px)', + }} + > + ~ + </div> + ) : ( + <Icon media={icon} /> + )}libs/shared/ui/tooltip/tooltip.tsx (2)
20-20: Extract animation classes for better maintainability.The animation classes could be extracted into a constant or utility function.
Consider extracting the classes:
+const tooltipAnimationClasses = + 'animate-in fade-in-0 zoom-in-95 data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=closed]:zoom-out-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2' + +const tooltipBaseClasses = + 'z-50 overflow-hidden rounded-md bg-primary px-3 py-1.5 text-xs text-primary-foreground' + className={cn( - 'z-50 overflow-hidden rounded-md bg-primary px-3 py-1.5 text-xs text-primary-foreground animate-in fade-in-0 zoom-in-95 data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=closed]:zoom-out-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2', + tooltipBaseClasses, + tooltipAnimationClasses, className, )}
29-29: Export TypeScript types for better developer experience.Consider exporting the component prop types for better TypeScript integration.
Add type exports:
+export type TooltipContentProps = React.ComponentPropsWithoutRef<typeof TooltipPrimitive.Content> +export type TooltipTriggerProps = React.ComponentPropsWithoutRef<typeof TooltipPrimitive.Trigger> export { Tooltip, TooltipContent, TooltipProvider, TooltipTrigger }libs/external/shadcn/components/ui/tooltip/tooltip.tsx (1)
11-27: Consider adding accessibility attributes.While the implementation is solid, consider enhancing accessibility by adding
aria-labeloraria-describedbyattributes to the tooltip content.<TooltipPrimitive.Content ref={ref} sideOffset={sideOffset} + aria-label={typeof props.children === 'string' ? props.children : undefined} className={cn( 'z-50 overflow-hidden rounded-md bg-primary px-3 py-1.5 text-xs text-primary-foreground animate-in fade-in-0 zoom-in-95 data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=closed]:zoom-out-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2', className, )} {...props} />libs/portal/ui/dashboard/hackathon-countdown/hackathon-countdown.tsx (1)
29-29: Fix duplicate className.The
w-16class is duplicated withw-full, which might cause confusion.- <GlassmorphicCard key={dateType} className="w-16 gap-1-y p-3 flex items-center justify-center flex-col w-full"> + <GlassmorphicCard key={dateType} className="gap-1-y p-3 flex items-center justify-center flex-col w-full">libs/shared/ui/logo/logo.tsx (2)
19-19: Enhance image accessibility.Consider adding width and height attributes to prevent layout shifts and improve Core Web Vitals.
- <img src={cuHackingLogo} alt="CuHacking Logo" className="max-h-6 max-w-6 rounded-sm hover:scale-110 duration-300" /> + <img + src={cuHackingLogo} + alt="CuHacking Logo" + width={24} + height={24} + className="max-h-6 max-w-6 rounded-sm hover:scale-110 duration-300" + />
23-38: Simplify markup structure.The nested p tag inside motion.span is unnecessary and can be simplified.
- <motion.span + <motion.p initial={{ opacity: 0 }} animate={{ opacity: 1 }} transition={{ duration: 0.5 }} - className="font-medium text-black dark:text-white whitespace-pre" + className="font-medium text-foreground whitespace-pre" > - <p className="text-foreground"> cuHacking - </p> - </motion.span> + </motion.p>libs/external/shadcn/components/ui/tooltip/tooltip.stories.tsx (1)
29-30: Improve type safety in render function.The
anytype for args should be replaced with a proper type definition.- render: (args: any) => ( + render: ({ side, content }: { side?: 'top' | 'bottom' | 'left' | 'right', content?: string }) => (libs/shared/ui/tooltip/tooltip.stories.tsx (1)
29-29: Consider using a more specific type for render args.Instead of using
any, consider creating a specific type for the args to improve type safety.- render: (args: any) => ( + render: (args: { side: 'top' | 'bottom' | 'left' | 'right'; content: string }) => (libs/portal/ui/dashboard/sidebar/sidebar.tsx (2)
15-19: Add discriminator to SidebarContextProps interface.The interface should include a discriminator to ensure type safety when checking context existence.
interface SidebarContextProps { + __type: 'SidebarContext' open: boolean setOpen: React.Dispatch<React.SetStateAction<boolean>> animate: boolean }
25-32: Consider memoizing the context value.The context value is recreated on every render. Consider using
useMemoto optimize performance.export function useSidebar() { const context = useContext(SidebarContext) if (!context) { throw new Error('useSidebar must be used within a SidebarProvider') } + return useMemo(() => context, [context]) - return context }libs/portal/ui/dashboard/sidebar/sidebar.stories.tsx (1)
15-39: Add accessibility parameters to story metadata.Include a11y addon parameters to ensure accessibility testing in Storybook.
const meta = { title: '🌀 Portal/Sidebar', component: Sidebar, tags: ['autodocs'], parameters: { layout: 'fullscreen', backgrounds: { default: 'dark', }, + a11y: { + config: { + rules: [ + { + id: 'color-contrast', + enabled: true + } + ] + } + } },libs/portal/pages/index/index.tsx (1)
38-94: Extract navigation links to a separate configuration file.The navigation links array should be moved to a separate configuration file for better maintainability.
Create a new file
navigation.config.ts:export const navigationLinks = [ { label: 'Events', href: '/events', icon: Calender, }, // ... other links ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
libs/portal/assets/public/Calender.pngis excluded by!**/*.pnglibs/portal/assets/public/cuhackingnavlogo.pngis excluded by!**/*.pnglibs/portal/assets/public/hacker-mode.pngis excluded by!**/*.pnglibs/portal/assets/public/handshake.pngis excluded by!**/*.pnglibs/portal/assets/public/map.pngis excluded by!**/*.pnglibs/portal/assets/public/mountain.pngis excluded by!**/*.pnglibs/portal/assets/public/user.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (32)
.storybook/main.ts(1 hunks)apps/portal/app/routes/_index.tsx(1 hunks)libs/external/shadcn/components/ui/tooltip/tooltip.stories.tsx(1 hunks)libs/external/shadcn/components/ui/tooltip/tooltip.tsx(1 hunks)libs/portal/pages/index/index.tsx(1 hunks)libs/portal/ui/dashboard/banner/banner.stories.tsx(1 hunks)libs/portal/ui/dashboard/banner/banner.tsx(1 hunks)libs/portal/ui/dashboard/banner/index.ts(1 hunks)libs/portal/ui/dashboard/hackathon-countdown/countdown.ts(1 hunks)libs/portal/ui/dashboard/hackathon-countdown/hackathon-countdown.stories.tsx(1 hunks)libs/portal/ui/dashboard/hackathon-countdown/hackathon-countdown.tsx(1 hunks)libs/portal/ui/dashboard/hackathon-countdown/index.ts(1 hunks)libs/portal/ui/dashboard/legal/index.ts(1 hunks)libs/portal/ui/dashboard/legal/legal.stories.tsx(1 hunks)libs/portal/ui/dashboard/legal/legal.tsx(1 hunks)libs/portal/ui/dashboard/sidebar/lib/utils.ts(1 hunks)libs/portal/ui/dashboard/sidebar/sidebar.stories.tsx(1 hunks)libs/portal/ui/dashboard/sidebar/sidebar.tsx(1 hunks)libs/portal/ui/dashboard/stat/index.ts(1 hunks)libs/portal/ui/dashboard/stat/stat.stories.tsx(1 hunks)libs/portal/ui/dashboard/stat/stat.tsx(1 hunks)libs/portal/ui/dashboard/user-status/index.ts(1 hunks)libs/portal/ui/dashboard/user-status/user-status.stories.tsx(1 hunks)libs/portal/ui/dashboard/user-status/user-status.tsx(1 hunks)libs/shared/ui/icon/icon.tsx(1 hunks)libs/shared/ui/logo/index.tsx(1 hunks)libs/shared/ui/logo/logo.stories.tsx(1 hunks)libs/shared/ui/logo/logo.tsx(1 hunks)libs/shared/ui/terminal-text/terminal-text.tsx(2 hunks)libs/shared/ui/tooltip/tooltip.stories.tsx(1 hunks)libs/shared/ui/tooltip/tooltip.tsx(1 hunks)package.json(2 hunks)
✅ Files skipped from review due to trivial changes (10)
- libs/shared/ui/logo/index.tsx
- libs/portal/ui/dashboard/stat/index.ts
- libs/portal/ui/dashboard/hackathon-countdown/index.ts
- libs/portal/ui/dashboard/legal/index.ts
- libs/shared/ui/icon/icon.tsx
- libs/portal/ui/dashboard/user-status/index.ts
- libs/portal/ui/dashboard/user-status/user-status.stories.tsx
- libs/portal/ui/dashboard/banner/index.ts
- libs/portal/ui/dashboard/hackathon-countdown/hackathon-countdown.stories.tsx
- libs/portal/ui/dashboard/stat/stat.stories.tsx
🔇 Additional comments (11)
libs/portal/ui/dashboard/stat/stat.tsx (1)
1-2: LGTM! Good use of shared components.The imports are well-structured and leverage shared UI components, promoting code reusability.
libs/portal/ui/dashboard/sidebar/lib/utils.ts (1)
1-7: LGTM! Consider adding tests for the utility function.The implementation follows best practices by combining
clsxandtailwind-mergefor robust class name handling. Since this utility is likely used across multiple components, consider adding unit tests to ensure reliability.Here's a script to check the usage of this utility function across the codebase:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check usage of the cn utility function rg -A 2 "import.*cn.*from.*utils" --type ts --type tsxLength of output: 84
I'll generate a revised search script using file globbing (with the -g flag) instead of built-in file types. This should overcome the tsx issue:
Please run this new command to verify the usage of the
cnutility function. Once we see the output (or lack thereof), we can make a more definitive conclusion.
🏁 Script executed:
[run_scripts] #!/bin/bash # Description: Check usage of the cn utility function in ts and tsx files using file globbing. rg -A 2 "import.*cn.*from.*utils" -g "*.ts" -g "*.tsx"Length of output: 16291
Utility Function Implementation Verified – Tests Are Recommended
The
cnfunction correctly integratesclsxwithtailwind-mergeand is widely used throughout the codebase. Given its broad usage, adding unit tests is a valuable next step to ensure its continued reliability and easy maintenance in the future.
- The implementation follows best practices.
- Usage across numerous files confirms its critical role.
- Adding tests will help catch any regressions from potential future changes in dependencies.
libs/external/shadcn/components/ui/tooltip/tooltip.tsx (1)
1-10: LGTM! Clean re-exports of Radix UI components.The implementation follows best practices by re-exporting Radix UI components directly.
libs/portal/ui/dashboard/hackathon-countdown/hackathon-countdown.tsx (1)
6-11: LGTM! Clean interface definition.The Countdown interface is well-structured and uses appropriate types.
libs/shared/ui/logo/logo.tsx (1)
6-11: LGTM! Well-structured props interface.The LogoProps interface clearly defines all required and optional properties.
libs/external/shadcn/components/ui/tooltip/tooltip.stories.tsx (1)
41-60: LGTM! Clean story variations.The position variations effectively demonstrate different tooltip placements while maintaining DRY principles by reusing the Default story's render function.
libs/shared/ui/tooltip/tooltip.stories.tsx (1)
1-60: Well-structured Storybook configuration!The stories effectively demonstrate all tooltip positions and provide interactive controls for customization.
libs/shared/ui/logo/logo.stories.tsx (1)
1-67: Excellent Storybook configuration with proper Remix integration!The stories effectively cover all logo variations and properly handle routing with RemixStub.
libs/portal/ui/dashboard/user-status/user-status.tsx (1)
39-43: Reconsider using Rick Roll link in production.While humorous, a Rick Roll link might not be appropriate for a production environment. Consider using a more professional placeholder or actual documentation link.
.storybook/main.ts (1)
20-20: LGTM!The new path for portal UI stories follows the established pattern and maintains consistency with existing configuration.
package.json (1)
73-73: Verify compatibility with Node.js 22.13.1.Please ensure that the new dependencies
@radix-ui/[email protected]and[email protected]are compatible with Node.js 22.13.1.Also applies to: 92-92
✅ Verification successful
🌐 Web query:
Are @radix-ui/[email protected] and [email protected] compatible with Node.js 22.13.1?💡 Result:
Based on the search results:
Compatibility Status:
@radix-ui/[email protected] and [email protected] appear compatible with Node.js 22.13.1 based on available evidence.Key Analysis:
1️⃣ Node.js 22.x Compatibility
- Node.js 22 entered LTS status in October 2024[2][8]
- Version 22.13.1 contains security fixes but maintains backward compatibility[4][6][8]
- No breaking changes reported for npm packages in security updates[6]
2️⃣ @radix-ui/react-tooltip
- Latest version 1.1.8 released Feb 2025[3]
- Actively maintained with recent dependency updates[7]
- Installation instructions confirm standard npm/yarn workflow[3][5]
3️⃣ Motion Library Considerations
- While [email protected] isn't explicitly mentioned in sources:
- Node.js 22's ES module support (stable since v22) handles modern packages[2][8]
- Security patches in v22 don't affect standard React dependencies[6]
Recommendations:
# Recommended install command npm install @radix-ui/[email protected] [email protected]Verification Steps:
1️⃣ Clear package manager cache if experiencing issues:npm cache clean --force2️⃣ Restart TypeScript server after installation[1]
3️⃣ Check for dependency conflicts using:npm ls @radix-ui/react-tooltip motionNo incompatibility reports were found between these specific versions in the analyzed sources[1-10]. For production use, test in a staging environment first following Node.js LTS best practices[2][8].
Citations:
- 1: Cannot find module '@radix-ui/react-tooltip' or its corresponding type declarations. radix-ui/primitives#827
- 2: https://nodejs.org/en/blog/announcements/v22-release-announce
- 3: https://www.npmjs.com/package/@radix-ui/react-tooltip
- 4: https://github.com/nodejs/node/blob/main/doc/changelogs/CHANGELOG_V22.md
- 5: https://www.radix-ui.com/primitives/docs/components/tooltip
- 6: https://nodejs.org/en/blog/release/v22.13.1
- 7: https://www.radix-ui.com/primitives/docs/overview/releases
- 8: https://nodejs.org/en
- 9: https://www.radix-ui.com/themes/docs/overview/releases
- 10: https://www.npmjs.com/package/@radix-ui/react-tooltip/v/1.1.8-rc.1
Compatibility Verified:
Both @radix-ui/[email protected] and [email protected] have been confirmed as compatible with Node.js 22.13.1 based on the available evidence. No changes are needed regarding these dependencies. For thoroughness, testing in a staging environment is still recommended.
| <GlassmorphicCard className="gap-y-1 p-3 text-center w-auto"> | ||
| <p>{label}</p> | ||
| <Typography variant="h2" className="text-center">{value}</Typography> | ||
| </GlassmorphicCard> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and prop validation.
The component could benefit from:
- ARIA attributes for better screen reader support
- Prop validation for required fields
- Configurable typography variant
- <GlassmorphicCard className="gap-y-1 p-3 text-center w-auto">
- <p>{label}</p>
- <Typography variant="h2" className="text-center">{value}</Typography>
+ <GlassmorphicCard
+ className="gap-y-1 p-3 text-center w-auto"
+ role="region"
+ aria-label={`Statistic for ${label}`}
+ >
+ <p className="text-sm text-gray-600">{label}</p>
+ <Typography
+ variant="h2"
+ className="text-center"
+ aria-live="polite"
+ >
+ {value}
+ </Typography>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <GlassmorphicCard className="gap-y-1 p-3 text-center w-auto"> | |
| <p>{label}</p> | |
| <Typography variant="h2" className="text-center">{value}</Typography> | |
| </GlassmorphicCard> | |
| <GlassmorphicCard | |
| className="gap-y-1 p-3 text-center w-auto" | |
| role="region" | |
| aria-label={`Statistic for ${label}`} | |
| > | |
| <p className="text-sm text-gray-600">{label}</p> | |
| <Typography | |
| variant="h2" | |
| className="text-center" | |
| aria-live="polite" | |
| > | |
| {value} | |
| </Typography> | |
| </GlassmorphicCard> |
| export function HackathonCountdown({ date }: { date: Date }) { | ||
| const [countdown, setCountdown] = useState<Countdown>(getCountdownTo(date)) | ||
| useEffect(() => { | ||
| const interval = setInterval(() => { | ||
| setCountdown(getCountdownTo(date)) | ||
| }, 1000) | ||
|
|
||
| return () => clearInterval(interval) | ||
| }, [date]) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for past dates.
The countdown logic should handle cases where the target date is in the past to avoid negative values.
export function HackathonCountdown({ date }: { date: Date }) {
+ const isDatePast = date.getTime() < Date.now()
const [countdown, setCountdown] = useState<Countdown>(getCountdownTo(date))
useEffect(() => {
+ if (isDatePast) {
+ setCountdown({ days: 0, hrs: 0, mins: 0, secs: 0 })
+ return
+ }
const interval = setInterval(() => {
setCountdown(getCountdownTo(date))
}, 1000)
return () => clearInterval(interval)
}, [date])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function HackathonCountdown({ date }: { date: Date }) { | |
| const [countdown, setCountdown] = useState<Countdown>(getCountdownTo(date)) | |
| useEffect(() => { | |
| const interval = setInterval(() => { | |
| setCountdown(getCountdownTo(date)) | |
| }, 1000) | |
| return () => clearInterval(interval) | |
| }, [date]) | |
| export function HackathonCountdown({ date }: { date: Date }) { | |
| const isDatePast = date.getTime() < Date.now() | |
| const [countdown, setCountdown] = useState<Countdown>(getCountdownTo(date)) | |
| useEffect(() => { | |
| if (isDatePast) { | |
| setCountdown({ days: 0, hrs: 0, mins: 0, secs: 0 }) | |
| return | |
| } | |
| const interval = setInterval(() => { | |
| setCountdown(getCountdownTo(date)) | |
| }, 1000) | |
| return () => clearInterval(interval) | |
| }, [date]) |
| <Button | ||
| className="w-full text-black" | ||
| onClick={() => {}} | ||
| variant="secondary" | ||
| > | ||
| RSVP | ||
| </Button> |
There was a problem hiding this comment.
Implement RSVP button functionality.
The RSVP button has an empty onClick handler which could lead to user confusion.
- onClick={() => {}}
+ onClick={() => {
+ // TODO: Implement RSVP functionality
+ console.warn('RSVP functionality not implemented');
+ }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Button | |
| className="w-full text-black" | |
| onClick={() => {}} | |
| variant="secondary" | |
| > | |
| RSVP | |
| </Button> | |
| <Button | |
| className="w-full text-black" | |
| onClick={() => { | |
| // TODO: Implement RSVP functionality | |
| console.warn('RSVP functionality not implemented'); | |
| }} | |
| variant="secondary" | |
| > | |
| RSVP | |
| </Button> |
| switch (status) { | ||
| case 'accepted': | ||
| messageContent = 'WELCOME TO CUHACKING 2025! 💚' | ||
| additionalContent = ( | ||
| <> | ||
|
|
||
| <TerminalText className="text-orange-400">!!!ACCEPTED!!!</TerminalText> | ||
| <TerminalText>WE HOPE TO CUHACKING!</TerminalText> | ||
| <Button | ||
| className="w-full text-black" | ||
| onClick={() => {}} | ||
| variant="secondary" | ||
| > | ||
| RSVP | ||
| </Button> | ||
| </> | ||
| ) | ||
| break | ||
| case 'pending': | ||
| messageContent = 'HI! THANK YOU FOR APPLYING 💚' | ||
| additionalContent = ( | ||
|
|
||
| <TerminalText> | ||
| <a href="https://www.youtube.com/watch?v=dQw4w9WgXcQ" target="_blank" rel="noopener noreferrer"> | ||
| Under review 👀 | ||
| </a> | ||
| </TerminalText> | ||
|
|
||
| ) | ||
| break | ||
| case 'rejected': | ||
| additionalContent = ( | ||
| <> | ||
| <TerminalText>Unfortunately, we couldn't accept your application this time.</TerminalText> | ||
| <TerminalText>Don't be discouraged! We encourage you to apply again next year. 💪</TerminalText> | ||
| </> | ||
| ) | ||
| break | ||
| } |
There was a problem hiding this comment.
Add default case and message content for rejected status.
The switch statement is missing a default case and the rejected status doesn't set messageContent.
switch (status) {
case 'accepted':
messageContent = 'WELCOME TO CUHACKING 2025! 💚'
// ... rest of the case
break
case 'pending':
messageContent = 'HI! THANK YOU FOR APPLYING 💚'
// ... rest of the case
break
case 'rejected':
+ messageContent = 'THANK YOU FOR YOUR INTEREST 💚'
additionalContent = (
// ... rest of the case
)
break
+ default:
+ messageContent = 'WELCOME TO CUHACKING 2025! 💚'
+ additionalContent = null
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch (status) { | |
| case 'accepted': | |
| messageContent = 'WELCOME TO CUHACKING 2025! 💚' | |
| additionalContent = ( | |
| <> | |
| <TerminalText className="text-orange-400">!!!ACCEPTED!!!</TerminalText> | |
| <TerminalText>WE HOPE TO CUHACKING!</TerminalText> | |
| <Button | |
| className="w-full text-black" | |
| onClick={() => {}} | |
| variant="secondary" | |
| > | |
| RSVP | |
| </Button> | |
| </> | |
| ) | |
| break | |
| case 'pending': | |
| messageContent = 'HI! THANK YOU FOR APPLYING 💚' | |
| additionalContent = ( | |
| <TerminalText> | |
| <a href="https://www.youtube.com/watch?v=dQw4w9WgXcQ" target="_blank" rel="noopener noreferrer"> | |
| Under review 👀 | |
| </a> | |
| </TerminalText> | |
| ) | |
| break | |
| case 'rejected': | |
| additionalContent = ( | |
| <> | |
| <TerminalText>Unfortunately, we couldn't accept your application this time.</TerminalText> | |
| <TerminalText>Don't be discouraged! We encourage you to apply again next year. 💪</TerminalText> | |
| </> | |
| ) | |
| break | |
| } | |
| switch (status) { | |
| case 'accepted': | |
| messageContent = 'WELCOME TO CUHACKING 2025! 💚' | |
| additionalContent = ( | |
| <> | |
| <TerminalText className="text-orange-400">!!!ACCEPTED!!!</TerminalText> | |
| <TerminalText>WE HOPE TO CUHACKING!</TerminalText> | |
| <Button | |
| className="w-full text-black" | |
| onClick={() => {}} | |
| variant="secondary" | |
| > | |
| RSVP | |
| </Button> | |
| </> | |
| ) | |
| break | |
| case 'pending': | |
| messageContent = 'HI! THANK YOU FOR APPLYING 💚' | |
| additionalContent = ( | |
| <TerminalText> | |
| <a href="https://www.youtube.com/watch?v=dQw4w9WgXcQ" target="_blank" rel="noopener noreferrer"> | |
| Under review 👀 | |
| </a> | |
| </TerminalText> | |
| ) | |
| break | |
| case 'rejected': | |
| messageContent = 'THANK YOU FOR YOUR INTEREST 💚' | |
| additionalContent = ( | |
| <> | |
| <TerminalText>Unfortunately, we couldn't accept your application this time.</TerminalText> | |
| <TerminalText>Don't be discouraged! We encourage you to apply again next year. 💪</TerminalText> | |
| </> | |
| ) | |
| break | |
| default: | |
| messageContent = 'WELCOME TO CUHACKING 2025! 💚' | |
| additionalContent = null | |
| } |
| export function SidebarBody(props: React.ComponentProps<typeof motion.div>) { | ||
| return ( | ||
| <> | ||
| <DesktopSidebar {...props} /> | ||
| <MobileSidebar {...(props as React.ComponentProps<'div'>)} /> | ||
| </> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA attributes for accessibility.
The sidebar body should have appropriate ARIA attributes for better accessibility.
export function SidebarBody(props: React.ComponentProps<typeof motion.div>) {
return (
<>
- <DesktopSidebar {...props} />
- <MobileSidebar {...(props as React.ComponentProps<'div'>)} />
+ <DesktopSidebar {...props} role="navigation" aria-label="Main navigation" />
+ <MobileSidebar {...(props as React.ComponentProps<'div'>)} role="navigation" aria-label="Mobile navigation" />
</>
)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function SidebarBody(props: React.ComponentProps<typeof motion.div>) { | |
| return ( | |
| <> | |
| <DesktopSidebar {...props} /> | |
| <MobileSidebar {...(props as React.ComponentProps<'div'>)} /> | |
| </> | |
| ) | |
| } | |
| export function SidebarBody(props: React.ComponentProps<typeof motion.div>) { | |
| return ( | |
| <> | |
| <DesktopSidebar {...props} role="navigation" aria-label="Main navigation" /> | |
| <MobileSidebar {...(props as React.ComponentProps<'div'>)} role="navigation" aria-label="Mobile navigation" /> | |
| </> | |
| ) | |
| } |
| export function SidebarLink({ | ||
| link, | ||
| className, | ||
| ...props | ||
| }: { | ||
| link: Links | ||
| className?: string | ||
| props?: LinkProps | ||
| }) { | ||
| const { open, animate } = useSidebar() | ||
| return ( | ||
| <Link | ||
| to={link.href} | ||
| className={cn( | ||
| 'flex items-center gap-2 py-2 rounded-md transition-all duration-300 text-neutral-700 dark:text-neutral-200 hover:bg-neutral-100 dark:hover:bg-neutral-700 group', | ||
| className, | ||
| )} | ||
| {...props} | ||
| > | ||
| {link.icon} | ||
|
|
||
| <motion.span | ||
| animate={{ | ||
| display: animate ? (open ? 'inline-block' : 'none') : 'inline-block', | ||
| opacity: animate ? (open ? 1 : 0) : 1, | ||
| }} | ||
| className="text-neutral-700 dark:text-neutral-200 text-sm group-hover/sidebar:translate-x-1 transition duration-150 whitespace-pre inline-block !p-0 !m-0" | ||
| > | ||
| {link.label} | ||
| </motion.span> | ||
| </Link> | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyboard navigation support.
The SidebarLink component should support keyboard navigation for better accessibility.
export function SidebarLink({
link,
className,
...props
}: {
link: Links
className?: string
props?: LinkProps
}) {
const { open, animate } = useSidebar()
return (
<Link
to={link.href}
+ role="menuitem"
+ tabIndex={0}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault()
+ e.currentTarget.click()
+ }
+ }}
className={cn(
'flex items-center gap-2 py-2 rounded-md transition-all duration-300 text-neutral-700 dark:text-neutral-200 hover:bg-neutral-100 dark:hover:bg-neutral-700 group',
className,
)}
{...props}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function SidebarLink({ | |
| link, | |
| className, | |
| ...props | |
| }: { | |
| link: Links | |
| className?: string | |
| props?: LinkProps | |
| }) { | |
| const { open, animate } = useSidebar() | |
| return ( | |
| <Link | |
| to={link.href} | |
| className={cn( | |
| 'flex items-center gap-2 py-2 rounded-md transition-all duration-300 text-neutral-700 dark:text-neutral-200 hover:bg-neutral-100 dark:hover:bg-neutral-700 group', | |
| className, | |
| )} | |
| {...props} | |
| > | |
| {link.icon} | |
| <motion.span | |
| animate={{ | |
| display: animate ? (open ? 'inline-block' : 'none') : 'inline-block', | |
| opacity: animate ? (open ? 1 : 0) : 1, | |
| }} | |
| className="text-neutral-700 dark:text-neutral-200 text-sm group-hover/sidebar:translate-x-1 transition duration-150 whitespace-pre inline-block !p-0 !m-0" | |
| > | |
| {link.label} | |
| </motion.span> | |
| </Link> | |
| ) | |
| } | |
| export function SidebarLink({ | |
| link, | |
| className, | |
| ...props | |
| }: { | |
| link: Links | |
| className?: string | |
| props?: LinkProps | |
| }) { | |
| const { open, animate } = useSidebar() | |
| return ( | |
| <Link | |
| to={link.href} | |
| role="menuitem" | |
| tabIndex={0} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault() | |
| e.currentTarget.click() | |
| } | |
| }} | |
| className={cn( | |
| 'flex items-center gap-2 py-2 rounded-md transition-all duration-300 text-neutral-700 dark:text-neutral-200 hover:bg-neutral-100 dark:hover:bg-neutral-700 group', | |
| className, | |
| )} | |
| {...props} | |
| > | |
| {link.icon} | |
| <motion.span | |
| animate={{ | |
| display: animate ? (open ? 'inline-block' : 'none') : 'inline-block', | |
| opacity: animate ? (open ? 1 : 0) : 1, | |
| }} | |
| className="text-neutral-700 dark:text-neutral-200 text-sm group-hover/sidebar:translate-x-1 transition duration-150 whitespace-pre inline-block !p-0 !m-0" | |
| > | |
| {link.label} | |
| </motion.span> | |
| </Link> | |
| ) | |
| } |
libs/portal/pages/index/index.tsx
Outdated
| const constants = { | ||
| user: { | ||
| name: 'Saim', | ||
| }, | ||
| hackathonDate: new Date(2025, 3, 14), | ||
| status: 'pending', | ||
| stats: [ | ||
| { | ||
| label: 'Total Applications', | ||
| value: 1000, | ||
| }, | ||
| { | ||
| label: 'Money Raised', | ||
| value: '$12,230', | ||
| }, | ||
| ], | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move constants to environment configuration.
Hardcoded user data and dates should be moved to environment configuration or fetched from an API.
Consider creating a configuration file or environment variables for these values:
-const constants = {
- user: {
- name: 'Saim',
- },
- hackathonDate: new Date(2025, 3, 14),
- status: 'pending',
- stats: [
- {
- label: 'Total Applications',
- value: 1000,
- },
- {
- label: 'Money Raised',
- value: '$12,230',
- },
- ],
-}
+import { config } from '@cuhacking/shared/config'
+const { user, hackathonDate, status, stats } = config📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const constants = { | |
| user: { | |
| name: 'Saim', | |
| }, | |
| hackathonDate: new Date(2025, 3, 14), | |
| status: 'pending', | |
| stats: [ | |
| { | |
| label: 'Total Applications', | |
| value: 1000, | |
| }, | |
| { | |
| label: 'Money Raised', | |
| value: '$12,230', | |
| }, | |
| ], | |
| } | |
| import { config } from '@cuhacking/shared/config' | |
| const { user, hackathonDate, status, stats } = config |
| const legalData = [ | ||
| { | ||
| value: 'MLH Code of Conduct', | ||
| title: 'MLH Code of Conduct', | ||
| content: `Additional cases of harassment include, but are not limited to, sharing sexual images, deliberate intimidation, stalking, following, brigading, doxxing, harassing photography or recording, sustained disruption of talks or other events, inappropriate physical contact, and unwelcome sexual attention. If what you’re doing is making someone feel uncomfortable, that counts as harassment and is enough reason to stop doing it. | ||
|
|
||
| Participants asked to stop any harassing behavior are expected to comply immediately. Sponsors, judges, mentors, volunteers, organizers, MLH staff, and anyone else participating in the event are also subject to the anti-harassment policy. In particular, attendees should not use sexualized images, activities, or other material both in their hacks and during the event. Booth staff (including volunteers) should not use sexualized clothing, uniforms, or costumes, or otherwise create a sexualized environment. | ||
|
|
||
| If a participant engages in harassing behavior, MLH may take any action it deems appropriate, including warning the offender or expulsion from the event with no eligibility for reimbursement or refund of any type. If you are being harassed, notice that someone else is being harassed, or have any other concerns, please contact MLH using the reporting procedures defined below. | ||
|
|
||
| MLH representatives can help participants contact campus security or local law enforcement, provide escorts, or otherwise assist those experiencing harassment to feel safe for the duration of the event. We value your attendance. | ||
|
|
||
| We expect participants to follow these rules at all hackathon venues, online interactions in relation to the event, hackathon-related social events, and on hackathon-supplied transportation.`, | ||
| button_content: 'I have read and agree to the MLH Code of Conduct', | ||
| }, | ||
| { | ||
| value: 'MLH Terms & Conditions', | ||
| title: 'MLH Terms & Conditions', | ||
| content: 'These are the terms and conditions of MLH.', | ||
| button_content: 'I have read and agree to the MLH Terms & Conditions', | ||
| }, | ||
| { | ||
| value: 'MLH Privacy Policy', | ||
| title: 'MLH Privacy Policy', | ||
| content: 'Here is how MLH handles your privacy and data.', | ||
| button_content: 'I have read and agree to the MLH Privacy Policy', | ||
| }, | ||
| { | ||
| value: 'cuHacking Terms & Conditions', | ||
| title: 'cuHacking Terms & Conditions', | ||
| content: 'The terms and conditions specific to cuHacking are outlined here.', | ||
| button_content: 'I have read and agree to the cuHacking Terms & Conditions', | ||
| }, | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move legal content to a separate configuration file.
Sensitive legal content should be moved to a separate configuration file and potentially fetched from an API.
Create a new file legal.config.ts and move the content there:
export const legalDocuments = [
{
value: 'MLH Code of Conduct',
title: 'MLH Code of Conduct',
content: '...',
button_content: '...',
},
// ... other documents
]| <Button | ||
| onClick={() => {}} | ||
| className={allRead} | ||
| > | ||
| LETS CREATE YOUR PROFILE | ||
| </Button> | ||
| </div> |
There was a problem hiding this comment.
Fix className and add error handling for profile creation.
The Button component has an invalid className and lacks error handling.
<Button
- onClick={() => {}}
+ onClick={async () => {
+ try {
+ await createProfile()
+ } catch (error) {
+ console.error('Failed to create profile:', error)
+ // Show error notification to user
+ }
+ }}
- className={allRead}
+ className={`${allRead ? 'opacity-100' : 'opacity-50'} px-4 py-2 rounded-lg`}
+ disabled={!allRead}
>
LETS CREATE YOUR PROFILE
</Button>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Button | |
| onClick={() => {}} | |
| className={allRead} | |
| > | |
| LETS CREATE YOUR PROFILE | |
| </Button> | |
| </div> | |
| <Button | |
| onClick={async () => { | |
| try { | |
| await createProfile() | |
| } catch (error) { | |
| console.error('Failed to create profile:', error) | |
| // Show error notification to user | |
| } | |
| }} | |
| className={`${allRead ? 'opacity-100' : 'opacity-50'} px-4 py-2 rounded-lg`} | |
| disabled={!allRead} | |
| > | |
| LETS CREATE YOUR PROFILE | |
| </Button> | |
| </div> |
HasithDeAlwis
force-pushed
the
hasith/feat/278-portal-create-dashboard
branch
2 times, most recently
from
436a2ef to
e87897a
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Nitpick comments (12)
libs/portal/ui/dashboard/hackathon-countdown/countdown.ts (3)
8-20: Improve readability and maintainability of time calculations.Consider extracting magic numbers into named constants and using a more functional approach to calculate time units.
+ const MS_PER_SECOND = 1000 + const SECONDS_PER_MINUTE = 60 + const MINUTES_PER_HOUR = 60 + const HOURS_PER_DAY = 24 + + const MS_PER_MINUTE = MS_PER_SECOND * SECONDS_PER_MINUTE + const MS_PER_HOUR = MS_PER_MINUTE * MINUTES_PER_HOUR + const MS_PER_DAY = MS_PER_HOUR * HOURS_PER_DAY + - let diff = date.getTime() - now.getTime() + const totalMs = date.getTime() - now.getTime() - const days = Math.floor(diff / (1000 * 60 * 60 * 24)) - diff -= days * 1000 * 60 * 60 * 24 + const days = Math.floor(totalMs / MS_PER_DAY) + const remainingMs = totalMs % MS_PER_DAY - const hrs = Math.floor(diff / (1000 * 60 * 60)) - diff -= hrs * 1000 * 60 * 60 + const hrs = Math.floor(remainingMs / MS_PER_HOUR) + const remainingAfterHours = remainingMs % MS_PER_HOUR - const mins = Math.floor(diff / (1000 * 60)) - diff -= mins * 1000 * 60 + const mins = Math.floor(remainingAfterHours / MS_PER_MINUTE) + const remainingAfterMinutes = remainingAfterHours % MS_PER_MINUTE - const secs = Math.floor(diff / 1000) + const secs = Math.floor(remainingAfterMinutes / MS_PER_SECOND)
1-1: Add JSDoc documentation for better code maintainability.Consider adding comprehensive documentation to improve code clarity and maintainability.
+/** + * Calculates the time remaining until a specified future date. + * @param date - The target future date to count down to + * @returns An object containing the breakdown of time remaining + * @property {number} days - Number of full days remaining + * @property {number} hrs - Number of hours remaining after days + * @property {number} mins - Number of minutes remaining after hours + * @property {number} secs - Number of seconds remaining after minutes + * @throws {Error} If the target date is null, invalid, or in the past + */ export function getCountdownTo(date: Date): { days: number, hrs: number, mins: number, secs: number } {Also applies to: 21-22
1-22: Consider performance optimization for React integration.Since this function might be called frequently in the HackathonCountdown component (e.g., every second), consider memoizing the calculation or using performance.now() for more precise timing.
+const getTimeMs = () => performance.now() + export function getCountdownTo(date: Date): { days: number, hrs: number, mins: number, secs: number } { - const now = new Date() + const nowMs = getTimeMs() + const targetMs = date.getTime()Additionally, consider using React.useMemo in the component to prevent unnecessary recalculations:
// In HackathonCountdown.tsx const countdown = React.useMemo( () => getCountdownTo(targetDate), [targetDate, /* update interval */] )libs/shared/ui/sidebar/sidebar.tsx (4)
25-56: Use stable context values for performance.You are creating a new context value object on every render, potentially causing unnecessary re-renders in descendant components. Consider using a
useMemoor storing constant values outside the component to avoid passing unstable context values:export function SidebarProvider({ children, open: openProp, setOpen: setOpenProp, animate = true, }: { children: React.ReactNode open?: boolean setOpen?: React.Dispatch<React.SetStateAction<boolean>> animate?: boolean }) { const [openState, setOpenState] = useState(false) const open = openProp !== undefined ? openProp : openState const setOpen = setOpenProp !== undefined ? setOpenProp : setOpenState - return ( - <SidebarContext.Provider value={{ open, setOpen, animate }}> - {children} - </SidebarContext.Provider> - ) + const contextValue = useMemo(() => ({ open, setOpen, animate }), [open, setOpen, animate]) + return ( + <SidebarContext.Provider value={contextValue}> + {children} + </SidebarContext.Provider> + ) }
76-83: Align types between DesktopSidebar & MobileSidebar.
SidebarBodyspreads props for amotion.divontoDesktopSidebarbut casts them to a standarddivforMobileSidebar. This might cause confusing or unexpected behavior. Consider a single, consistent prop type or separate typed props for each variation.
112-116: Add accessibility attributes for the toggle button.Your toggle button in
MobileSidebarlacks ARIA attributes indicating whether the sidebar is open. Consider addingaria-expandedandaria-controlsto improve accessibility:<ButtonIcon className="text-neutral-200" - onClick={() => setOpen(!open)} + aria-expanded={open} + aria-controls="mobile-sidebar" + onClick={() => setOpen(!open)} />
162-225: Refine disabled link approach for accessibility and consistent tooltip behavior.
- Add
aria-disabled="true"when disabling a link and consider using hover/focus instead of click for showing the tooltip.- Toggling display from
'none'to'inline-block'in the motion props can cause layout shifts, which may be jarring for users. Consider a more subtle animation or fixed-width container.libs/portal/ui/sidebar/index.tsx (2)
16-58: Fix alt text mismatch in link icons.The
alttext does not reflect the actual icon usage (e.g., “Logout Icon” for a profile icon). This negatively affects accessibility. Update thealtattributes to accurately describe each icon:{ label: 'Profile', href: '/profile', disabled: false, icon: ( <img className="h-6 w-6" - src={user} - alt="Logout Icon" + src={user} + alt="Profile Icon" /> ), }(Apply similar changes to lines referencing handshake, mountain, and map.)
10-60: Extract link array to a shared config.You’re defining a static list of links inline. For improved maintainability, consider storing it in a separate config or constants file, especially if these links change frequently or require dynamic logic.
libs/shared/ui/sidebar/sidebar.stories.tsx (3)
1-14: Consider addressing ESLint warnings instead of disabling them.The ESLint disable comments suggest underlying code quality issues. Consider:
- Using destructuring where applicable instead of disabling
react/prefer-destructuring-assignment- Restructuring the code to avoid
ts/no-use-before-definewarningsAdditionally, consider using a constants file for image paths to improve maintainability.
119-139: Improve Logo component's accessibility and navigation.The Logo component needs some improvements:
- Add proper href instead of "#"
- Add proper aria labels
- Consider adding a title attribute to the link
<a - href="#" + href="/" + aria-label="Return to homepage" + title="CuHacking Homepage" className="flex items-center space-x-2 text-sm text-black py-1 relative z-20" >
141-204: Improve data structure and accessibility for navigation links.Several improvements needed:
- Inconsistent alt texts (e.g., "Settings Icon" for mountain.src)
- Hardcoded user name should come from a context or prop
- Consider adding active state handling for current route
const links = [ { label: 'Events', href: '#', + isActive: false, icon: ( <img className="h-5 w-5" src={Calender.src} - alt="Calendar Icon" + alt="View Events" /> ), }, // ... similar changes for other links ] -const constants = { - user: { - name: 'Saim', - }, -} +interface SidebarProps { + userName: string; +}
🛑 Comments failed to post (2)
libs/portal/ui/dashboard/hackathon-countdown/countdown.ts (1)
1-6: 🛠️ Refactor suggestion
Enhance input validation for robustness.
The current validation only checks if the date is in the future. Consider adding checks for:
- null/undefined date parameter
- invalid Date objects (e.g., NaN dates)
export function getCountdownTo(date: Date): { days: number, hrs: number, mins: number, secs: number } { + if (!date) { + throw new Error('Target date is required.') + } + if (isNaN(date.getTime())) { + throw new Error('Invalid target date provided.') + } const now = new Date() if (now > date) { - throw new Error('Target date must be in the future.') + throw new Error(`Target date (${date.toISOString()}) must be after current time (${now.toISOString()}).`) }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.export function getCountdownTo(date: Date): { days: number, hrs: number, mins: number, secs: number } { if (!date) { throw new Error('Target date is required.') } if (isNaN(date.getTime())) { throw new Error('Invalid target date provided.') } const now = new Date() if (now > date) { throw new Error(`Target date (${date.toISOString()}) must be after current time (${now.toISOString()}).`) } // ... remaining function implementation }libs/shared/ui/sidebar/sidebar.stories.tsx (1)
45-117: 🛠️ Refactor suggestion
Enhance accessibility for sidebar navigation.
While the implementation is functional, consider these accessibility improvements:
- Add
aria-labelto navigation links- Include
role="navigation"for the sidebar- Add keyboard navigation support
- Consider users who prefer reduced motion
- <Sidebar {...args} open={open} setOpen={setOpen}> + <Sidebar {...args} open={open} setOpen={setOpen} role="navigation" aria-label="Main"> <SidebarBody className="justify-between gap-10"> <div className="flex flex-col flex-1 overflow-y-auto overflow-x-hidden">And for motion animations:
+const shouldReduceMotion = window.matchMedia('(prefers-reduced-motion: reduce)').matches; <motion.a key={link.label} href={link.href} + aria-label={link.label} className="flex items-center gap-2 py-2 text-sm text-neutral-700 dark:text-neutral-200 hover:bg-neutral-100 dark:hover:bg-neutral-700 rounded-md w-full" - whileHover={{ scale: 1.02 }} + whileHover={shouldReduceMotion ? {} : { scale: 1.02 }} transition={{ type: 'spring', stiffness: 400, damping: 10 }} >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const Template: Story = (args: { open: boolean, animate: boolean }) => { const [open, setOpen] = useState(args.open) const shouldReduceMotion = window.matchMedia('(prefers-reduced-motion: reduce)').matches; useEffect(() => { setOpen(args.open) }, [args.open]) return ( <Sidebar {...args} open={open} setOpen={setOpen} role="navigation" aria-label="Main"> <SidebarBody className="justify-between gap-10"> <div className="flex flex-col flex-1 overflow-y-auto overflow-x-hidden"> <Logo /> <div className="mt-8 flex flex-col gap-2"> {links.map(link => ( <motion.a key={link.label} href={link.href} aria-label={link.label} className="flex items-center gap-2 py-2 text-sm text-neutral-700 dark:text-neutral-200 hover:bg-neutral-100 dark:hover:bg-neutral-700 rounded-md w-full" whileHover={shouldReduceMotion ? {} : { scale: 1.02 }} transition={{ type: 'spring', stiffness: 400, damping: 10 }} > {link.icon} <span>{link.label}</span> </motion.a> ))} </div> </div> <div> <motion.a href="#" className="flex items-center gap-2 py-2 text-sm text-neutral-700 dark:text-neutral-200 hover:bg-neutral-100 dark:hover:bg-neutral-700 rounded-md" whileHover={{ scale: 1.02 }} transition={{ type: 'spring', stiffness: 400, damping: 10 }} > <img src={hackermode.src} className="h-5 w-5" alt="Settings Icon" /> <span className={`truncate font-medium transition-all ${open ? 'visible opacity-100' : 'invisible opacity-0'}`} > Hacker Mode </span> </motion.a> <motion.a href="#" className="flex items-center gap-2 py-2 text-sm text-neutral-700 dark:text-neutral-200 hover:bg-neutral-100 dark:hover:bg-neutral-700 rounded-md" whileHover={{ scale: 1.02 }} transition={{ type: 'spring', stiffness: 400, damping: 10 }} > <img src={user.src} className="h-7 w-7 rounded-full" alt="Avatar" /> <span className={`truncate font-medium transition-all ${open ? 'visible opacity-100' : 'invisible opacity-0'}`} > {constants.user.name} </span> </motion.a> </div> </SidebarBody> </Sidebar> ) } export const Default: Story = Template.bind({}) Default.args = { open: true, animate: true, }
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
libs/portal/pages/index/index.tsx (2)
33-38: Make button text configurable.The button text "APPLY TO CUHACKING 6" should be moved to constants or configuration to make it easier to update.
+const constants = { + buttonText: 'APPLY TO CUHACKING 6', // ... other constants } <Button className="text-black px-8 md:px-12 py-2.5" variant="secondary" > - APPLY TO CUHACKING 6 + {constants.buttonText} </Button>
41-43: Add type safety to stats mapping.The stats array should have proper TypeScript interfaces to ensure type safety.
interface Stat { label: string; value: string | number; } const constants = { stats: [] as Stat[], // ... other constants }libs/shared/ui/sidebar/sidebar.tsx (4)
34-56: Re-evaluate context value stability.You’re disabling
react/no-unstable-context-value. This is fine for simple stateful logic, but it could lead to performance overhead or unnecessary re-renders in more complex scenarios. Consider using a memoized context value if you plan to expand logic in this provider.
85-110: Consider accessibility attributes for hover-triggered open state.Auto-expanding the desktop sidebar on hover can be disorienting for screen reader users. As a future improvement, you might add ARIA attributes (e.g.,
aria-expanded) or alternative ways to keep the sidebar open for assistive technologies.
112-160: Add an accessible label to the mobile toggle button.The mobile toggle uses an icon. Adding an
aria-labelor screen reader text helps ensure an inclusive user experience.<ButtonIcon + aria-label="Toggle sidebar" className="text-neutral-200" onClick={() => setOpen(!open)} />
1-226: Add unit test coverage for the entire sidebar feature.This new file implements key UI logic (mobile vs. desktop states, link handling, animations, etc.), but no tests are provided. Consider adding integration or snapshot tests to ensure that these components behave as expected across screen sizes.
Would you like a generated test suite to jumpstart coverage?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
libs/shared/assets/icons/general/handshake-white-1.svgis excluded by!**/*.svglibs/shared/assets/icons/general/map-white-1.svgis excluded by!**/*.svglibs/shared/assets/icons/general/mountain-white-1.svgis excluded by!**/*.svglibs/shared/assets/icons/general/profile-white-1.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (35)
.storybook/main.ts(1 hunks)apps/portal/app/routes/_index.tsx(1 hunks)libs/external/shadcn/components/ui/tooltip/tooltip.stories.tsx(1 hunks)libs/external/shadcn/components/ui/tooltip/tooltip.tsx(1 hunks)libs/portal/pages/index/index.tsx(1 hunks)libs/portal/ui/dashboard/banner/banner.stories.tsx(1 hunks)libs/portal/ui/dashboard/banner/banner.tsx(1 hunks)libs/portal/ui/dashboard/banner/index.ts(1 hunks)libs/portal/ui/dashboard/hackathon-countdown/countdown.ts(1 hunks)libs/portal/ui/dashboard/hackathon-countdown/hackathon-countdown.stories.tsx(1 hunks)libs/portal/ui/dashboard/hackathon-countdown/hackathon-countdown.tsx(1 hunks)libs/portal/ui/dashboard/hackathon-countdown/index.ts(1 hunks)libs/portal/ui/dashboard/legal/index.ts(1 hunks)libs/portal/ui/dashboard/legal/legal.stories.tsx(1 hunks)libs/portal/ui/dashboard/legal/legal.tsx(1 hunks)libs/portal/ui/dashboard/stat/index.ts(1 hunks)libs/portal/ui/dashboard/stat/stat.stories.tsx(1 hunks)libs/portal/ui/dashboard/stat/stat.tsx(1 hunks)libs/portal/ui/dashboard/user-status/index.ts(1 hunks)libs/portal/ui/dashboard/user-status/user-status.stories.tsx(1 hunks)libs/portal/ui/dashboard/user-status/user-status.tsx(1 hunks)libs/portal/ui/sidebar/index.tsx(1 hunks)libs/shared/ui/button/button.stories.tsx(1 hunks)libs/shared/ui/icon/icon.tsx(1 hunks)libs/shared/ui/logo/index.tsx(1 hunks)libs/shared/ui/logo/logo.stories.tsx(1 hunks)libs/shared/ui/logo/logo.tsx(1 hunks)libs/shared/ui/sidebar/index.tsx(1 hunks)libs/shared/ui/sidebar/sidebar.stories.tsx(1 hunks)libs/shared/ui/sidebar/sidebar.tsx(1 hunks)libs/shared/ui/terminal-text/terminal-text.tsx(2 hunks)libs/shared/ui/tooltip/index.ts(1 hunks)libs/shared/ui/tooltip/tooltip.stories.tsx(1 hunks)libs/shared/ui/tooltip/tooltip.tsx(1 hunks)package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (33)
- libs/portal/ui/dashboard/banner/index.ts
- libs/portal/ui/dashboard/stat/index.ts
- libs/portal/ui/dashboard/hackathon-countdown/index.ts
- libs/shared/ui/icon/icon.tsx
- apps/portal/app/routes/_index.tsx
- libs/portal/ui/dashboard/stat/stat.tsx
- libs/portal/ui/dashboard/user-status/index.ts
- libs/portal/ui/dashboard/user-status/user-status.stories.tsx
- package.json
- libs/portal/ui/dashboard/legal/legal.stories.tsx
- libs/portal/ui/dashboard/stat/stat.stories.tsx
- libs/external/shadcn/components/ui/tooltip/tooltip.tsx
- libs/portal/ui/dashboard/hackathon-countdown/hackathon-countdown.tsx
- libs/portal/ui/dashboard/hackathon-countdown/countdown.ts
- libs/portal/ui/dashboard/user-status/user-status.tsx
- libs/external/shadcn/components/ui/tooltip/tooltip.stories.tsx
- libs/shared/ui/tooltip/tooltip.stories.tsx
- libs/shared/ui/logo/logo.stories.tsx
- libs/shared/ui/logo/index.tsx
- libs/shared/ui/button/button.stories.tsx
- libs/portal/ui/dashboard/legal/legal.tsx
- libs/shared/ui/terminal-text/terminal-text.tsx
- libs/portal/ui/dashboard/legal/index.ts
- libs/shared/ui/sidebar/index.tsx
- libs/shared/ui/tooltip/index.ts
- .storybook/main.ts
- libs/portal/ui/dashboard/banner/banner.tsx
- libs/portal/ui/sidebar/index.tsx
- libs/shared/ui/sidebar/sidebar.stories.tsx
- libs/portal/ui/dashboard/banner/banner.stories.tsx
- libs/shared/ui/tooltip/tooltip.tsx
- libs/portal/ui/dashboard/hackathon-countdown/hackathon-countdown.stories.tsx
- libs/shared/ui/logo/logo.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: test
🔇 Additional comments (8)
libs/portal/pages/index/index.tsx (3)
1-6: LGTM! Well-organized imports.The imports follow a consistent pattern and are properly scoped under the appropriate packages.
8-24: Move constants to environment configuration.Hardcoded user data and dates should be moved to environment configuration or fetched from an API.
28-30: LGTM! Clean layout structure.The layout uses appropriate flex classes for responsive design and proper spacing.
libs/shared/ui/sidebar/sidebar.tsx (5)
9-13: No issues found in theLinksinterface.
15-19: Interface naming is clear and self-explanatory.
21-32: Custom hook usage looks good.The
useSidebarhook properly ensures that it is consumed within the provider, preventing usage errors. Nicely done.
58-74: Great approach for wrapping children in aSidebarProvider.
76-84: Straightforward composition of Desktop and Mobile sidebars.The
SidebarBodyeffectively unifies both desktop and mobile experiences. The implementation is clear and maintainable.
| user: { | ||
| name: 'Saim', | ||
| }, | ||
| hackathonDate: new Date(2025, 2, 14), |
There was a problem hiding this comment.
Fix the hackathon date initialization.
The JavaScript Date constructor uses 0-based month indexing (0-11). Currently, new Date(2025, 2, 14) will create a date for March 14, 2025. If you intended for February 14, 2025, you should use:
- hackathonDate: new Date(2025, 2, 14),
+ hackathonDate: new Date(2025, 1, 14),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| hackathonDate: new Date(2025, 2, 14), | |
| hackathonDate: new Date(2025, 1, 14), |
libs/shared/ui/sidebar/sidebar.tsx
Outdated
| import type { LinkProps } from 'next/link' | ||
| import { Tooltip, TooltipContent, TooltipProvider, TooltipTrigger } from '@cuhacking/shared/ui/tooltip' | ||
| import { cn } from '@cuhacking/shared/utils/cn' | ||
| import { ButtonIcon } from '@radix-ui/react-icons' | ||
| import { Link } from '@remix-run/react' | ||
| import { AnimatePresence, motion } from 'framer-motion' | ||
| import React, { createContext, useContext, useState } from 'react' |
There was a problem hiding this comment.
Consider aligning imports for a unified routing approach.
You’re importing LinkProps from 'next/link' while simultaneously using <Link> from '@remix-run/react'. These libraries have different routing semantics and incompatible prop signatures. To avoid confusion and potential type clashes, consider removing the next/link import or switching everything to a consistent framework.
libs/shared/ui/sidebar/sidebar.tsx
Outdated
| export function SidebarLink({ | ||
| link, | ||
| className, | ||
| disabled, | ||
| ...props | ||
| }: { | ||
| link: Links | ||
| className?: string | ||
| disabled?: boolean | ||
| props?: LinkProps | ||
| }) { | ||
| const { open, animate } = useSidebar() | ||
| const [openTooltip, setOpenTooltip] = useState<boolean>(false) | ||
| return ( | ||
| <> | ||
| {disabled | ||
| ? ( | ||
| <TooltipProvider> | ||
| <Tooltip delayDuration={300} open={openTooltip} onOpenChange={setOpenTooltip}> | ||
| <TooltipTrigger asChild className="w-full"> | ||
| <span | ||
| onClick={() => setOpenTooltip(prev => !prev)} | ||
| className={cn( | ||
| 'max-w-fit flex items-center gap-2 py-2 rounded-md text-neutral-400 dark:text-neutral-500 cursor-not-allowed', | ||
| className, | ||
| )} | ||
| {...props} | ||
| > | ||
| {link.icon} | ||
| <span className="text-sm whitespace-pre inline-block !p-0 !m-0"> | ||
| {link.label} | ||
| </span> | ||
| </span> | ||
| </TooltipTrigger> | ||
| <TooltipContent side="right"> | ||
| Coming soon | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| </TooltipProvider> | ||
| ) | ||
| : ( | ||
| <Link | ||
| to={link.href} | ||
| className={cn( | ||
| 'flex items-center gap-2 py-2 rounded-md transition-all duration-300 text-neutral-200 hover:bg-neutral-700 group', | ||
| className, | ||
| )} | ||
| {...props} | ||
| > | ||
| {link.icon} | ||
| <motion.span | ||
| animate={{ | ||
| display: animate ? (open ? 'inline-block' : 'none') : 'inline-block', | ||
| opacity: animate ? (open ? 1 : 0) : 1, | ||
| }} | ||
| className="text-neutral-200 text-sm group-hover/sidebar:translate-x-1 transition duration-150 whitespace-pre inline-block !p-0 !m-0" | ||
| > | ||
| {link.label} | ||
| </motion.span> | ||
| </Link> | ||
| )} | ||
| </> | ||
| ) | ||
| } |
There was a problem hiding this comment.
Fix type mismatch between Next.js props and Remix Link.
Currently, you rely on LinkProps from 'next/link' while using <Link> from '@remix-run/react'. This can lead to runtime or type-checking issues. Consider referencing the Remix link props or creating a custom type. For example:
-import type { LinkProps } from 'next/link'
+import type { LinkProps } from '@remix-run/react'
-}: {
- link: Links
- className?: string
- disabled?: boolean
- props?: LinkProps
-}) {
+type SidebarLinkProps = Omit<LinkProps, 'to'> & {
+ link: Links
+ className?: string
+ disabled?: boolean
+};
+export function SidebarLink({
+ link,
+ className,
+ disabled,
+ ...props
+}: SidebarLinkProps) {
...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function SidebarLink({ | |
| link, | |
| className, | |
| disabled, | |
| ...props | |
| }: { | |
| link: Links | |
| className?: string | |
| disabled?: boolean | |
| props?: LinkProps | |
| }) { | |
| const { open, animate } = useSidebar() | |
| const [openTooltip, setOpenTooltip] = useState<boolean>(false) | |
| return ( | |
| <> | |
| {disabled | |
| ? ( | |
| <TooltipProvider> | |
| <Tooltip delayDuration={300} open={openTooltip} onOpenChange={setOpenTooltip}> | |
| <TooltipTrigger asChild className="w-full"> | |
| <span | |
| onClick={() => setOpenTooltip(prev => !prev)} | |
| className={cn( | |
| 'max-w-fit flex items-center gap-2 py-2 rounded-md text-neutral-400 dark:text-neutral-500 cursor-not-allowed', | |
| className, | |
| )} | |
| {...props} | |
| > | |
| {link.icon} | |
| <span className="text-sm whitespace-pre inline-block !p-0 !m-0"> | |
| {link.label} | |
| </span> | |
| </span> | |
| </TooltipTrigger> | |
| <TooltipContent side="right"> | |
| Coming soon | |
| </TooltipContent> | |
| </Tooltip> | |
| </TooltipProvider> | |
| ) | |
| : ( | |
| <Link | |
| to={link.href} | |
| className={cn( | |
| 'flex items-center gap-2 py-2 rounded-md transition-all duration-300 text-neutral-200 hover:bg-neutral-700 group', | |
| className, | |
| )} | |
| {...props} | |
| > | |
| {link.icon} | |
| <motion.span | |
| animate={{ | |
| display: animate ? (open ? 'inline-block' : 'none') : 'inline-block', | |
| opacity: animate ? (open ? 1 : 0) : 1, | |
| }} | |
| className="text-neutral-200 text-sm group-hover/sidebar:translate-x-1 transition duration-150 whitespace-pre inline-block !p-0 !m-0" | |
| > | |
| {link.label} | |
| </motion.span> | |
| </Link> | |
| )} | |
| </> | |
| ) | |
| } | |
| import type { LinkProps } from '@remix-run/react' | |
| type SidebarLinkProps = Omit<LinkProps, 'to'> & { | |
| link: Links | |
| className?: string | |
| disabled?: boolean | |
| }; | |
| export function SidebarLink({ | |
| link, | |
| className, | |
| disabled, | |
| ...props | |
| }: SidebarLinkProps) { | |
| const { open, animate } = useSidebar() | |
| const [openTooltip, setOpenTooltip] = useState<boolean>(false) | |
| return ( | |
| <> | |
| {disabled | |
| ? ( | |
| <TooltipProvider> | |
| <Tooltip delayDuration={300} open={openTooltip} onOpenChange={setOpenTooltip}> | |
| <TooltipTrigger asChild className="w-full"> | |
| <span | |
| onClick={() => setOpenTooltip(prev => !prev)} | |
| className={cn( | |
| 'max-w-fit flex items-center gap-2 py-2 rounded-md text-neutral-400 dark:text-neutral-500 cursor-not-allowed', | |
| className, | |
| )} | |
| {...props} | |
| > | |
| {link.icon} | |
| <span className="text-sm whitespace-pre inline-block !p-0 !m-0"> | |
| {link.label} | |
| </span> | |
| </span> | |
| </TooltipTrigger> | |
| <TooltipContent side="right"> | |
| Coming soon | |
| </TooltipContent> | |
| </Tooltip> | |
| </TooltipProvider> | |
| ) | |
| : ( | |
| <Link | |
| to={link.href} | |
| className={cn( | |
| 'flex items-center gap-2 py-2 rounded-md transition-all duration-300 text-neutral-200 hover:bg-neutral-700 group', | |
| className, | |
| )} | |
| {...props} | |
| > | |
| {link.icon} | |
| <motion.span | |
| animate={{ | |
| display: animate ? (open ? 'inline-block' : 'none') : 'inline-block', | |
| opacity: animate ? (open ? 1 : 0) : 1, | |
| }} | |
| className="text-neutral-200 text-sm group-hover/sidebar:translate-x-1 transition duration-150 whitespace-pre inline-block !p-0 !m-0" | |
| > | |
| {link.label} | |
| </motion.span> | |
| </Link> | |
| )} | |
| </> | |
| ) | |
| } |
HasithDeAlwis
force-pushed
the
hasith/feat/278-portal-create-dashboard
branch
from
e87897a to
c75099a
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
libs/portal/pages/index/index.tsx (1)
44-44: Improve readability of conditional rendering.The line is too long and combines multiple conditions. Consider breaking it down for better readability.
- { constants.hackathonApplicationStatus !== UserHackathonApplicationStatus.notComplete && <UserStatus name={constants.user.name} status={constants.status} /> } + {constants.hackathonApplicationStatus !== UserHackathonApplicationStatus.notComplete && ( + <UserStatus + name={constants.user.name} + status={constants.status} + /> + )}libs/shared/ui/sidebar/sidebar.tsx (3)
129-129: Fix ESLint error for arrow function parentheses.Remove unnecessary parentheses around the single function argument.
- onClick={() => setOpen((prev) => !prev)} + onClick={() => setOpen(prev => !prev)}🧰 Tools
🪛 ESLint
[error] 129-129: Unexpected parentheses around single function argument having a body with no curly braces.
(style/arrow-parens)
127-127: Extract z-index values to CSS custom properties.Hardcoded z-index values (101, 100) should be moved to CSS custom properties for better maintainability.
+/* Add to your CSS variables */ +:root { + --z-index-sidebar-button: 101; + --z-index-sidebar-content: 100; +} - <div className="flex justify-end z-[101] w-full p-6"> + <div className="flex justify-end z-[var(--z-index-sidebar-button)] w-full p-6"> - 'fixed h-full w-full inset-0 dark:bg-neutral-900 p-6 z-[100] flex flex-col justify-between', + 'fixed h-full w-full inset-0 dark:bg-neutral-900 p-6 z-[var(--z-index-sidebar-content)] flex flex-col justify-between',Also applies to: 148-148
143-146: Extract animation constants.Consider extracting animation duration and easing function to constants for consistency and reusability.
+const ANIMATION_CONFIG = { + duration: 0.3, + ease: 'easeInOut', +} as const; - transition={{ - duration: 0.3, - ease: 'easeInOut', - }} + transition={ANIMATION_CONFIG}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
libs/shared/assets/icons/general/handshake-white-1.svgis excluded by!**/*.svglibs/shared/assets/icons/general/map-white-1.svgis excluded by!**/*.svglibs/shared/assets/icons/general/mountain-white-1.svgis excluded by!**/*.svglibs/shared/assets/icons/general/profile-white-1.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
apps/portal/app/routes/_index.tsx(1 hunks)libs/portal/pages/index/index.tsx(1 hunks)libs/portal/types/user.ts(1 hunks)libs/portal/ui/dashboard/legal/index.ts(1 hunks)libs/portal/ui/dashboard/legal/legal.stories.tsx(1 hunks)libs/portal/ui/dashboard/legal/legal.tsx(1 hunks)libs/portal/ui/sidebar/index.tsx(1 hunks)libs/shared/ui/button/button.stories.tsx(1 hunks)libs/shared/ui/sidebar/index.tsx(1 hunks)libs/shared/ui/sidebar/sidebar.stories.tsx(1 hunks)libs/shared/ui/sidebar/sidebar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- libs/shared/ui/sidebar/index.tsx
- libs/portal/ui/dashboard/legal/index.ts
- libs/shared/ui/button/button.stories.tsx
- apps/portal/app/routes/_index.tsx
- libs/portal/ui/dashboard/legal/legal.stories.tsx
- libs/portal/ui/dashboard/legal/legal.tsx
- libs/portal/ui/sidebar/index.tsx
- libs/shared/ui/sidebar/sidebar.stories.tsx
🧰 Additional context used
🪛 ESLint
libs/shared/ui/sidebar/sidebar.tsx
[error] 129-129: Unexpected parentheses around single function argument having a body with no curly braces.
(style/arrow-parens)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: build (22.13.1)
🔇 Additional comments (4)
libs/portal/types/user.ts (1)
45-49: LGTM! Well-structured enum for hackathon application status.The new enum follows consistent naming patterns and provides clear, descriptive values for all possible states.
libs/portal/pages/index/index.tsx (2)
9-21: Move constants to environment configuration.Hardcoded user data and dates should be moved to environment configuration or fetched from an API.
13-13: Fix the hackathon date initialization.The JavaScript Date constructor uses 0-based month indexing (0-11). Currently,
new Date(2025, 2, 14)will create a date for March 14, 2025.libs/shared/ui/sidebar/sidebar.tsx (1)
1-1: Fix type mismatch between Next.js props and RemixLink.Currently, you rely on
LinkPropsfrom'next/link'while using<Link>from'@remix-run/react'.Also applies to: 6-6
HasithDeAlwis
force-pushed
the
hasith/feat/278-portal-create-dashboard
branch
from
c75099a to
3e076bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
libs/portal/ui/dashboard/user-status/user-status.tsx (1)
17-55: Consider refactoring the switch statement to use objects.The switch statement could be simplified using objects for better maintainability and readability.
Consider refactoring to:
- let additionalContent - let messageContent - - switch (status) { - case 'accepted': - messageContent = 'WELCOME TO CUHACKING 2025! 💚' - additionalContent = ( - <> - <TerminalText className="text-orange-400">!!!ACCEPTED!!!</TerminalText> - <TerminalText>WE HOPE TO CUHACKING!</TerminalText> - <Button - className="w-full text-black" - onClick={() => {}} - variant="secondary" - > - RSVP - </Button> - </> - ) - break - // ... other cases - } + const statusConfig = { + accepted: { + messageContent: 'WELCOME TO CUHACKING 2025! 💚', + additionalContent: ( + <> + <TerminalText className="text-orange-400">!!!ACCEPTED!!!</TerminalText> + <TerminalText>WE HOPE TO CUHACKING!</TerminalText> + <Button + className="w-full text-black" + onClick={() => {}} + variant="secondary" + > + RSVP + </Button> + </> + ) + }, + pending: { + messageContent: 'HI! THANK YOU FOR APPLYING 💚', + additionalContent: ( + <TerminalText> + <a href="https://www.youtube.com/watch?v=dQw4w9WgXcQ" target="_blank" rel="noopener noreferrer"> + Under review 👀 + </a> + </TerminalText> + ) + }, + rejected: { + messageContent: 'THANK YOU FOR YOUR INTEREST 💚', + additionalContent: ( + <> + <TerminalText>Unfortunately, we couldn't accept your application this time.</TerminalText> + <TerminalText>Don't be discouraged! We encourage you to apply again next year. 💪</TerminalText> + </> + ) + } + } + + const { messageContent, additionalContent } = statusConfig[status] ?? statusConfig.pendinglibs/shared/ui/sidebar/sidebar.stories.tsx (2)
107-130: Consider simplifying the Template function.The Template function could be simplified by removing the internal state management and using the args directly, as Storybook handles state updates automatically.
Apply this diff to simplify the implementation:
-function Template(args: { open: boolean, animate: boolean }) { - const [open, setOpen] = useState(args.open) - - useEffect(() => { - setOpen(args.open) - }, [args.open]) - - return ( - <SidebarContainer open={open} setOpen={setOpen}> +function Template({ open, animate }: { open: boolean, animate: boolean }) { + return ( + <SidebarContainer open={open} setOpen={() => {}}>
125-126: Remove empty div.The empty div serves no purpose and should be removed.
Apply this diff:
- <div> - </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
libs/shared/assets/icons/general/handshake-white-1.svgis excluded by!**/*.svglibs/shared/assets/icons/general/map-white-1.svgis excluded by!**/*.svglibs/shared/assets/icons/general/mountain-white-1.svgis excluded by!**/*.svglibs/shared/assets/icons/general/profile-white-1.svgis excluded by!**/*.svgpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (36)
.storybook/main.ts(1 hunks)apps/portal/app/routes/_index.tsx(1 hunks)libs/external/shadcn/components/ui/tooltip/tooltip.stories.tsx(1 hunks)libs/external/shadcn/components/ui/tooltip/tooltip.tsx(1 hunks)libs/portal/pages/index/index.tsx(1 hunks)libs/portal/types/user.ts(1 hunks)libs/portal/ui/dashboard/banner/banner.stories.tsx(1 hunks)libs/portal/ui/dashboard/banner/banner.tsx(1 hunks)libs/portal/ui/dashboard/banner/index.ts(1 hunks)libs/portal/ui/dashboard/hackathon-countdown/countdown.ts(1 hunks)libs/portal/ui/dashboard/hackathon-countdown/hackathon-countdown.stories.tsx(1 hunks)libs/portal/ui/dashboard/hackathon-countdown/hackathon-countdown.tsx(1 hunks)libs/portal/ui/dashboard/hackathon-countdown/index.ts(1 hunks)libs/portal/ui/dashboard/legal/index.ts(1 hunks)libs/portal/ui/dashboard/legal/legal.stories.tsx(1 hunks)libs/portal/ui/dashboard/legal/legal.tsx(1 hunks)libs/portal/ui/dashboard/stat/index.ts(1 hunks)libs/portal/ui/dashboard/stat/stat.stories.tsx(1 hunks)libs/portal/ui/dashboard/stat/stat.tsx(1 hunks)libs/portal/ui/dashboard/user-status/index.ts(1 hunks)libs/portal/ui/dashboard/user-status/user-status.stories.tsx(1 hunks)libs/portal/ui/dashboard/user-status/user-status.tsx(1 hunks)libs/portal/ui/sidebar/index.tsx(1 hunks)libs/shared/ui/button/button.stories.tsx(1 hunks)libs/shared/ui/icon/icon.tsx(1 hunks)libs/shared/ui/logo/index.tsx(1 hunks)libs/shared/ui/logo/logo.stories.tsx(1 hunks)libs/shared/ui/logo/logo.tsx(1 hunks)libs/shared/ui/sidebar/index.tsx(1 hunks)libs/shared/ui/sidebar/sidebar.stories.tsx(1 hunks)libs/shared/ui/sidebar/sidebar.tsx(1 hunks)libs/shared/ui/terminal-text/terminal-text.tsx(2 hunks)libs/shared/ui/tooltip/index.ts(1 hunks)libs/shared/ui/tooltip/tooltip.stories.tsx(1 hunks)libs/shared/ui/tooltip/tooltip.tsx(1 hunks)package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (33)
- libs/shared/ui/logo/index.tsx
- libs/shared/ui/tooltip/index.ts
- libs/portal/ui/dashboard/legal/index.ts
- libs/portal/ui/dashboard/hackathon-countdown/index.ts
- libs/shared/ui/sidebar/index.tsx
- libs/shared/ui/button/button.stories.tsx
- libs/portal/ui/dashboard/user-status/index.ts
- libs/portal/types/user.ts
- .storybook/main.ts
- libs/portal/ui/dashboard/stat/stat.tsx
- libs/portal/ui/dashboard/banner/banner.tsx
- libs/portal/ui/dashboard/banner/index.ts
- libs/shared/ui/icon/icon.tsx
- libs/shared/ui/terminal-text/terminal-text.tsx
- apps/portal/app/routes/_index.tsx
- libs/portal/ui/dashboard/stat/index.ts
- libs/portal/ui/dashboard/user-status/user-status.stories.tsx
- package.json
- libs/portal/ui/dashboard/hackathon-countdown/countdown.ts
- libs/portal/ui/sidebar/index.tsx
- libs/portal/ui/dashboard/hackathon-countdown/hackathon-countdown.stories.tsx
- libs/portal/pages/index/index.tsx
- libs/portal/ui/dashboard/banner/banner.stories.tsx
- libs/shared/ui/logo/logo.tsx
- libs/portal/ui/dashboard/stat/stat.stories.tsx
- libs/portal/ui/dashboard/legal/legal.tsx
- libs/portal/ui/dashboard/hackathon-countdown/hackathon-countdown.tsx
- libs/portal/ui/dashboard/legal/legal.stories.tsx
- libs/external/shadcn/components/ui/tooltip/tooltip.tsx
- libs/shared/ui/tooltip/tooltip.stories.tsx
- libs/external/shadcn/components/ui/tooltip/tooltip.stories.tsx
- libs/shared/ui/tooltip/tooltip.tsx
- libs/shared/ui/logo/logo.stories.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test
- GitHub Check: build (22.13.1)
🔇 Additional comments (7)
libs/portal/ui/dashboard/user-status/user-status.tsx (3)
17-55: Add default case and message content for rejected status.The switch statement is missing a default case and the rejected status doesn't set messageContent.
25-31: Implement RSVP button functionality.The RSVP button has an empty onClick handler which could lead to user confusion.
57-77: LGTM! Clean and well-structured render logic.The component's render logic is well-organized, using semantic HTML and following the project's UI patterns.
libs/shared/ui/sidebar/sidebar.stories.tsx (2)
63-101: LGTM! Well-structured Storybook configuration.The meta configuration is comprehensive, with proper layout settings, background configuration, and argument types. The Remix stub is correctly implemented for routing simulation.
132-136: LGTM! Well-structured story export.The Default story is correctly exported with appropriate initial arguments.
libs/shared/ui/sidebar/sidebar.tsx (2)
1-23: LGTM! Well-structured imports and interfaces.The imports are organized correctly, and the interfaces are well-defined. The code now consistently uses Remix for routing.
58-83: LGTM! Well-structured component composition.The Sidebar and SidebarBody components are well-organized, with proper prop handling and responsive design support.
| type Status = 'pending' | 'accepted' | 'rejected' | ||
|
|
||
| interface UserStatusProps { | ||
| name: string | ||
| status?: Status | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the status prop required.
The status prop is marked as optional, but the component's logic assumes it exists in the switch statement. This could lead to undefined behavior.
Apply this diff to make the status prop required:
interface UserStatusProps {
name: string
- status?: Status
+ status: Status
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type Status = 'pending' | 'accepted' | 'rejected' | |
| interface UserStatusProps { | |
| name: string | |
| status?: Status | |
| } | |
| type Status = 'pending' | 'accepted' | 'rejected' | |
| interface UserStatusProps { | |
| name: string | |
| status: Status | |
| } |
| <img | ||
| className="h-5 w-5" | ||
| src={user.src} | ||
| alt="Logout Icon" | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inconsistent alt text in icon images.
The alt text for icons doesn't match their corresponding labels, which could affect accessibility:
- "User" link has "Logout Icon" alt text
- "Team" link has "Profile Icon" alt text
- "Challenges" link has "Settings Icon" alt text
- "Map" link has "Logout Icon" alt text
Apply this diff to fix the alt text:
- alt="Logout Icon"
+ alt="User Icon"- alt="Profile Icon"
+ alt="Team Icon"- alt="Settings Icon"
+ alt="Challenges Icon"- alt="Logout Icon"
+ alt="Map Icon"Also applies to: 30-34, 42-46, 54-58
libs/shared/ui/sidebar/sidebar.tsx
Outdated
| <span | ||
| className={cn( | ||
| 'max-w-fit flex items-center gap-2 py-2 rounded-md text-neutral-400 dark:text-neutral-500 cursor-not-allowed', | ||
| className, | ||
| )} | ||
| {...props} | ||
| > |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA attributes to disabled links.
The disabled links should have proper ARIA attributes to indicate their state to screen readers.
Apply this diff:
<span
className={cn(
'max-w-fit flex items-center gap-2 py-2 rounded-md text-neutral-400 dark:text-neutral-500 cursor-not-allowed',
className,
)}
+ aria-disabled="true"
+ role="link"
{...props}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <span | |
| className={cn( | |
| 'max-w-fit flex items-center gap-2 py-2 rounded-md text-neutral-400 dark:text-neutral-500 cursor-not-allowed', | |
| className, | |
| )} | |
| {...props} | |
| > | |
| <span | |
| className={cn( | |
| 'max-w-fit flex items-center gap-2 py-2 rounded-md text-neutral-400 dark:text-neutral-500 cursor-not-allowed', | |
| className, | |
| )} | |
| aria-disabled="true" | |
| role="link" | |
| {...props} | |
| > |
| <button | ||
| type="button" | ||
| onClick={() => setOpen(prev => !prev)} | ||
| > | ||
| <img | ||
| src={open ? cross : hamburger} | ||
| className={`transition-transform duration-300 ${open ? 'rotate-90' : ''}`} | ||
| /> | ||
| </button> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add ARIA label to the toggle button.
The toggle button lacks an ARIA label, which affects accessibility for screen reader users.
Apply this diff:
<button
type="button"
+ aria-label={open ? "Close menu" : "Open menu"}
onClick={() => setOpen(prev => !prev)}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <button | |
| type="button" | |
| onClick={() => setOpen(prev => !prev)} | |
| > | |
| <img | |
| src={open ? cross : hamburger} | |
| className={`transition-transform duration-300 ${open ? 'rotate-90' : ''}`} | |
| /> | |
| </button> | |
| <button | |
| type="button" | |
| aria-label={open ? "Close menu" : "Open menu"} | |
| onClick={() => setOpen(prev => !prev)} | |
| > | |
| <img | |
| src={open ? cross : hamburger} | |
| className={`transition-transform duration-300 ${open ? 'rotate-90' : ''}`} | |
| /> | |
| </button> |
| // eslint-disable-next-line react/no-unstable-context-value | ||
| <SidebarContext.Provider value={{ open, setOpen, animate }}> | ||
| {children} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix unstable context value.
The ESLint warning about unstable context value is suppressed, but it's better to fix the underlying issue. The context value object is recreated on every render, which could cause unnecessary re-renders of consuming components.
Apply this diff to stabilize the context value:
+ const contextValue = React.useMemo(
+ () => ({ open, setOpen, animate }),
+ [open, setOpen, animate]
+ )
- <SidebarContext.Provider value={{ open, setOpen, animate }}>
+ <SidebarContext.Provider value={contextValue}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // eslint-disable-next-line react/no-unstable-context-value | |
| <SidebarContext.Provider value={{ open, setOpen, animate }}> | |
| {children} | |
| // eslint-disable-next-line react/no-unstable-context-value | |
| const contextValue = React.useMemo( | |
| () => ({ open, setOpen, animate }), | |
| [open, setOpen, animate] | |
| ); | |
| <SidebarContext.Provider value={contextValue}> | |
| {children} |
HasithDeAlwis
force-pushed
the
hasith/feat/278-portal-create-dashboard
branch
from
3e076bb to
122b3fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
libs/shared/ui/sidebar/sidebar.stories.tsx (2)
120-122: Add key prop based on a stable identifier.Using array index as key prop can lead to issues with component state and performance when the array items are reordered.
Apply this diff to use a stable identifier:
- <SidebarLink className="px-2" disabled={link.disabled} key={idx} link={link} /> + <SidebarLink className="px-2" disabled={link.disabled} key={link.label} link={link} />
107-130: Consider memoizing the Template component.The Template component is recreated on every render, which could affect performance in Storybook.
Apply this diff to memoize the component:
-function Template(args: { open: boolean, animate: boolean }) { +const Template = React.memo(function Template(args: { open: boolean, animate: boolean }) { const [open, setOpen] = useState(args.open) useEffect(() => { setOpen(args.open) }, [args.open]) return ( <SidebarContainer open={open} setOpen={setOpen}> <SidebarBody className="justify-between gap-10"> <div className="flex flex-col flex-1 gap-y-2 overflow-y-auto overflow-x-hidden"> <Logo className="px-2" link="/" hasWordmark hasAnimation /> <div className="flex flex-col gap-2"> {links.map((link, idx) => ( <SidebarLink className="px-2" disabled={link.disabled} key={idx} link={link} /> ))} </div> </div> <div> </div> </SidebarBody> </SidebarContainer> ) -} +})libs/shared/ui/sidebar/sidebar.tsx (1)
102-103: Consider user preferences for motion.The sidebar's auto-open behavior on hover might cause issues for users who prefer reduced motion.
Apply this diff to respect user preferences:
+ const prefersReducedMotion = window.matchMedia('(prefers-reduced-motion: reduce)').matches return ( <> <motion.div className={cn( 'h-full px-4 py-4 hidden md:flex md:flex-col w-[300px] flex-shrink-0', 'bg-[rgba(27,27,27,0.4)] backdrop-blur-md border-r border-[rgba(255,255,255,0.1)]', className, )} animate={{ - width: animate ? (open ? '300px' : '60px') : '300px', + width: animate && !prefersReducedMotion ? (open ? '300px' : '60px') : '300px', }} - onMouseEnter={() => setOpen(true)} - onMouseLeave={() => setOpen(false)} + onMouseEnter={() => !prefersReducedMotion && setOpen(true)} + onMouseLeave={() => !prefersReducedMotion && setOpen(false)} {...props} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
libs/shared/assets/icons/general/handshake-white-1.svgis excluded by!**/*.svglibs/shared/assets/icons/general/map-white-1.svgis excluded by!**/*.svglibs/shared/assets/icons/general/mountain-white-1.svgis excluded by!**/*.svglibs/shared/assets/icons/general/profile-white-1.svgis excluded by!**/*.svg
📒 Files selected for processing (11)
apps/portal/app/routes/_index.tsx(1 hunks)libs/portal/pages/index/index.tsx(1 hunks)libs/portal/types/user.ts(1 hunks)libs/portal/ui/dashboard/legal/index.ts(1 hunks)libs/portal/ui/dashboard/legal/legal.stories.tsx(1 hunks)libs/portal/ui/dashboard/legal/legal.tsx(1 hunks)libs/portal/ui/sidebar/index.tsx(1 hunks)libs/shared/ui/button/button.stories.tsx(1 hunks)libs/shared/ui/sidebar/index.tsx(1 hunks)libs/shared/ui/sidebar/sidebar.stories.tsx(1 hunks)libs/shared/ui/sidebar/sidebar.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- libs/shared/ui/sidebar/index.tsx
- libs/portal/ui/dashboard/legal/index.ts
- libs/portal/types/user.ts
- libs/shared/ui/button/button.stories.tsx
- libs/portal/ui/dashboard/legal/legal.stories.tsx
- libs/portal/pages/index/index.tsx
- apps/portal/app/routes/_index.tsx
- libs/portal/ui/dashboard/legal/legal.tsx
- libs/portal/ui/sidebar/index.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (22.13.1)
- GitHub Check: test
🔇 Additional comments (4)
libs/shared/ui/sidebar/sidebar.stories.tsx (1)
18-22: Fix inconsistent alt text in icon images.The alt text for icons doesn't match their corresponding labels, which could affect accessibility.
Apply this diff to fix the alt text:
- alt="Logout Icon" + alt="User Icon"- alt="Profile Icon" + alt="Team Icon"- alt="Settings Icon" + alt="Challenges Icon"- alt="Logout Icon" + alt="Map Icon"Also applies to: 30-34, 42-46, 54-58
libs/shared/ui/sidebar/sidebar.tsx (3)
51-53: Fix unstable context value.The ESLint warning about unstable context value is suppressed, but it's better to fix the underlying issue.
Apply this diff to stabilize the context value:
+ const contextValue = React.useMemo( + () => ({ open, setOpen, animate }), + [open, setOpen, animate] + ) - <SidebarContext.Provider value={{ open, setOpen, animate }}> + <SidebarContext.Provider value={contextValue}>
127-135: Add ARIA label to the toggle button.The toggle button lacks an ARIA label, which affects accessibility for screen reader users.
Apply this diff:
<button type="button" + aria-label={open ? "Close menu" : "Open menu"} onClick={() => setOpen(prev => !prev)} >
180-186: Add ARIA attributes to disabled links.The disabled links should have proper ARIA attributes to indicate their state to screen readers.
Apply this diff:
<div className={cn( 'max-w-fit flex items-center gap-2 rounded-md text-neutral-400 dark:text-neutral-500 cursor-not-allowed', className, )} + aria-disabled="true" + role="link" {...props} >
| <motion.div | ||
| initial={{ x: '-100%', opacity: 0 }} | ||
| animate={{ x: 0, opacity: 1 }} | ||
| exit={{ x: '-100%', opacity: 0 }} | ||
| transition={{ | ||
| duration: 0.3, | ||
| ease: 'easeInOut', | ||
| }} | ||
| className={cn( | ||
| 'fixed h-full w-full inset-0 dark:bg-neutral-900 p-6 z-[100] flex flex-col justify-between', | ||
| className, | ||
| )} | ||
| > | ||
| {children} | ||
| </motion.div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add focus trap for mobile sidebar.
When the mobile sidebar is open, focus should be trapped within it to improve accessibility.
Install and use the focus-trap-react package:
+import { FocusTrap } from 'focus-trap-react'
return (
<>
<AnimatePresence>
{open && (
+ <FocusTrap>
<motion.div
initial={{ x: '-100%', opacity: 0 }}
animate={{ x: 0, opacity: 1 }}
exit={{ x: '-100%', opacity: 0 }}
transition={{
duration: 0.3,
ease: 'easeInOut',
}}
className={cn(
'fixed h-full w-full inset-0 dark:bg-neutral-900 p-6 z-[100] flex flex-col justify-between',
className,
)}
>
{children}
</motion.div>
+ </FocusTrap>
)}
</AnimatePresence>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <motion.div | |
| initial={{ x: '-100%', opacity: 0 }} | |
| animate={{ x: 0, opacity: 1 }} | |
| exit={{ x: '-100%', opacity: 0 }} | |
| transition={{ | |
| duration: 0.3, | |
| ease: 'easeInOut', | |
| }} | |
| className={cn( | |
| 'fixed h-full w-full inset-0 dark:bg-neutral-900 p-6 z-[100] flex flex-col justify-between', | |
| className, | |
| )} | |
| > | |
| {children} | |
| </motion.div> | |
| import { FocusTrap } from 'focus-trap-react' | |
| ... | |
| return ( | |
| <> | |
| <AnimatePresence> | |
| {open && ( | |
| <FocusTrap> | |
| <motion.div | |
| initial={{ x: '-100%', opacity: 0 }} | |
| animate={{ x: 0, opacity: 1 }} | |
| exit={{ x: '-100%', opacity: 0 }} | |
| transition={{ | |
| duration: 0.3, | |
| ease: 'easeInOut', | |
| }} | |
| className={cn( | |
| 'fixed h-full w-full inset-0 dark:bg-neutral-900 p-6 z-[100] flex flex-col justify-between', | |
| className, | |
| )} | |
| > | |
| {children} | |
| </motion.div> | |
| </FocusTrap> | |
| )} | |
| </AnimatePresence> | |
| </> | |
| ) |
|
🎉 This PR is included in version 2.8.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Checklist
Summary by CodeRabbit
New Features
Bannercomponent to display welcoming messages.HackathonCountdowncomponent to show time remaining until events.UserStatuscomponent to reflect application statuses.Tooltipcomponent for enhanced user interactions.Sidebarcomponent for improved navigation experience.Statcomponent to display key metrics.Logocomponent for branding consistency.Legalcomponent to present legal documents for user acknowledgment.Chores