-
Notifications
You must be signed in to change notification settings - Fork 29
Fix dark mode hover issue and add theme toggle support #51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix dark mode hover issue and add theme toggle support #51
Conversation
WalkthroughThis PR introduces dark mode support by integrating next-themes for theme management, updating the header with responsive styling and a sticky border, simplifying the footer border styling, and adding a new ModeToggle component for theme switching. The ThemeProvider component's API is refactored to enforce fixed theme configuration. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
components/footer.tsx (1)
27-38: Missing dark mode styles in footer.The footer lacks dark mode variants, which is inconsistent with the dark mode support added elsewhere in this PR. Several elements will have poor contrast in dark mode:
- Line 27:
border-gray-200needsdark:border-zinc-800- Line 34:
text-gray-600needsdark:text-gray-400- Line 38:
text-gray-600needsdark:text-gray-400- <footer className="border-t border-gray-200 bg-gradient-to-r from-[#228B22]/5 via-[#91A511]/5 to-[#FFBF00]/5 backdrop-blur-sm mt-20"> + <footer className="border-t border-gray-200 dark:border-zinc-800 bg-gradient-to-r from-[#228B22]/5 via-[#91A511]/5 to-[#FFBF00]/5 dark:from-[#228B22]/10 dark:via-[#91A511]/10 dark:to-[#FFBF00]/10 backdrop-blur-sm mt-20">Similarly, update text colors with dark variants throughout the footer.
app/page.tsx (1)
78-97: Loading and error states lack dark mode support.The loading spinner and error message screens still use light-only colors (
from-green-50,via-yellow-50,text-gray-600), which will appear broken in dark mode.Apply dark mode variants to match the main content area:
- <div className="min-h-screen bg-gradient-to-br from-green-50 via-yellow-50 to-[#FFC517]/10 flex items-center justify-center"> + <div className="min-h-screen bg-gradient-to-br from-green-50 via-yellow-50 to-[#FFC517]/10 dark:from-zinc-900 dark:via-zinc-900 dark:to-black flex items-center justify-center"> <div className="text-center"> <div className="animate-spin rounded-full h-12 w-12 border-b-2 border-[#228B22] mx-auto mb-4"></div> - <p className="text-gray-600">Loading articles...</p> + <p className="text-gray-600 dark:text-zinc-400">Loading articles...</p> </div> </div>Apply the same pattern to the error state (lines 91-96).
🧹 Nitpick comments (1)
app/page.tsx (1)
10-10: Unused import:ModeToggleis imported but not rendered.The
ModeTogglecomponent is imported but commented out on line 140. Either render it or remove the import to avoid dead code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
app/page.tsx(2 hunks)components/footer.tsx(1 hunks)components/mode-toggle.tsx(1 hunks)components/theme-provider.tsx(1 hunks)package.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/mode-toggle.tsx (1)
components/ui/button.tsx (1)
Button(56-56)
🔇 Additional comments (3)
package.json (1)
12-19: LGTM!The new dependencies are appropriate for the dark mode feature:
next-themesis the standard solution for theme management in Next.js apps@radix-ui/react-slotsupports the Button component's composition patternapp/page.tsx (1)
102-161: LGTM on header dark mode implementation!The header correctly implements dark mode with appropriate border, background, and text color variants. The sticky positioning and backdrop blur provide a polished UX.
components/theme-provider.tsx (1)
5-15: LGTM!The
ThemeProviderconfiguration is correct:
attribute="class"aligns with Tailwind's dark mode strategydefaultTheme="system"respects user OS preferencesenableSystemenables automatic theme detectionConsider adding
disableTransitionOnChangeto prevent a brief flash of styles during theme transitions:<NextThemesProvider attribute="class" defaultTheme="system" enableSystem + disableTransitionOnChange >This is optional and depends on whether you observe any visual glitches during theme switching.
| export function ModeToggle() { | ||
| const { theme, setTheme } = useTheme(); | ||
| const [mounted, setMounted] = useState(false); | ||
|
|
||
| useEffect(() => setMounted(true), []); | ||
| if (!mounted) return null; | ||
|
|
||
| return ( | ||
| <Button | ||
| variant="ghost" | ||
| size="icon" | ||
| onClick={() => setTheme(theme === "dark" ? "light" : "dark")} | ||
| aria-label="Toggle theme" | ||
| > | ||
| {theme === "dark" ? ( | ||
| <Sun className="h-5 w-5" /> | ||
| ) : ( | ||
| <Moon className="h-5 w-5" /> | ||
| )} | ||
| </Button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use resolvedTheme instead of theme for accurate theme detection.
When defaultTheme is "system" (as configured in ThemeProvider), the theme value can be "system" rather than the actual resolved theme ("dark" or "light"). This causes incorrect icon display and toggle behavior.
export function ModeToggle() {
- const { theme, setTheme } = useTheme();
+ const { resolvedTheme, setTheme } = useTheme();
const [mounted, setMounted] = useState(false);
useEffect(() => setMounted(true), []);
if (!mounted) return null;
return (
<Button
variant="ghost"
size="icon"
- onClick={() => setTheme(theme === "dark" ? "light" : "dark")}
+ onClick={() => setTheme(resolvedTheme === "dark" ? "light" : "dark")}
aria-label="Toggle theme"
>
- {theme === "dark" ? (
+ {resolvedTheme === "dark" ? (
<Sun className="h-5 w-5" />
) : (
<Moon className="h-5 w-5" />
)}
</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.
| export function ModeToggle() { | |
| const { theme, setTheme } = useTheme(); | |
| const [mounted, setMounted] = useState(false); | |
| useEffect(() => setMounted(true), []); | |
| if (!mounted) return null; | |
| return ( | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| onClick={() => setTheme(theme === "dark" ? "light" : "dark")} | |
| aria-label="Toggle theme" | |
| > | |
| {theme === "dark" ? ( | |
| <Sun className="h-5 w-5" /> | |
| ) : ( | |
| <Moon className="h-5 w-5" /> | |
| )} | |
| </Button> | |
| export function ModeToggle() { | |
| const { resolvedTheme, setTheme } = useTheme(); | |
| const [mounted, setMounted] = useState(false); | |
| useEffect(() => setMounted(true), []); | |
| if (!mounted) return null; | |
| return ( | |
| <Button | |
| variant="ghost" | |
| size="icon" | |
| onClick={() => setTheme(resolvedTheme === "dark" ? "light" : "dark")} | |
| aria-label="Toggle theme" | |
| > | |
| {resolvedTheme === "dark" ? ( | |
| <Sun className="h-5 w-5" /> | |
| ) : ( | |
| <Moon className="h-5 w-5" /> | |
| )} | |
| </Button> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In components/mode-toggle.tsx around lines 8 to 27, the component uses `theme`
which can be the literal "system" when defaultTheme is "system", causing
incorrect icon and toggle behavior; switch to using `resolvedTheme` from
useTheme for accurate current theme detection (use it for rendering the Sun/Moon
icon) and use `setTheme(resolvedTheme === "dark" ? "light" : "dark")` for the
onClick toggle; keep the mounted guard and import/useResolvedTheme as provided
by next-themes (or destructure `resolvedTheme` from useTheme), and ensure you
fall back safely if `resolvedTheme` is undefined.
This PR resolves a dark mode UI issue where article titles became invisible
on hover due to insufficient contrast.
Changes included:
These changes improve accessibility, consistency, and overall user experience.
Fixes #15
Summary by CodeRabbit
Release Notes
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.