feat (client-demo): add theme toggle menu#1545
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR adds theme switching functionality to the Home component by importing the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 24544570158Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage remained the same at 41.816%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
web/apps/client-demo/src/pages/Home.tsx (2)
297-316: MovethemeOptionsout of the component body.Since this list is static and doesn't depend on any props/state, defining it at module scope avoids recreating the array (and re-rendering the inline icon elements) on every render.
♻️ Proposed refactor
+const THEME_OPTIONS = [ + { key: 'light', label: 'Light', icon: <SunIcon />, testId: 'theme-light-option' }, + { key: 'dark', label: 'Dark', icon: <MoonIcon />, testId: 'theme-dark-option' }, + { key: 'system', label: 'System', icon: <DesktopIcon />, testId: 'theme-system-option' }, +] as const; + export default function Home() { ... - const themeOptions = [ - { key: 'light', label: 'Light', icon: <SunIcon />, testId: 'theme-light-option' }, - { key: 'dark', label: 'Dark', icon: <MoonIcon />, testId: 'theme-dark-option' }, - { key: 'system', label: 'System', icon: <DesktopIcon />, testId: 'theme-system-option' }, - ] as const;
369-378: Consider a radio/checkmark pattern instead ofdisabledto indicate the active theme.Using
disabledon the currently active theme makes it unclear whether the item is unavailable or simply selected, and it also prevents re-triggeringsetTheme(a minor UX quirk). ADropdownMenu.RadioGroup/RadioItem(or a trailing checkmark) communicates selection state more clearly and is more accessible. Keep the current approach if it's an intentional constraint of the apsaraDropdownMenuAPI.Does `@raystack/apsara` DropdownMenu support RadioGroup/RadioItem or a selected/checked state?
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba463a45-f103-4e16-9c49-c6d8ff48e2ba
📒 Files selected for processing (1)
web/apps/client-demo/src/pages/Home.tsx
Summary
feat (client-demo): add theme toggle menu in the root page
Changes
Technical Details
Test Plan