-
Notifications
You must be signed in to change notification settings - Fork 29
Fix: theme toggle button and fix theme color #77
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?
Conversation
📝 WalkthroughWalkthroughThis PR introduces dark mode support across the application by creating a new ThemeToggleButton component and applying dark mode styling (dark: variants) to multiple pages and components including the homepage, submit page, footer, and pagination controls. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 2
🤖 Fix all issues with AI agents
In @components/pagination.tsx:
- Line 147: The className string in the pagination component contains a
concatenated class token ("dark:bg-gray-900text-gray-400") causing a CSS syntax
error; edit the className (the disabled control or button element where
className is set in components/pagination.tsx) and insert a space between
"dark:bg-gray-900" and "text-gray-400" so the two Tailwind classes are separate
and apply correctly.
🧹 Nitpick comments (2)
components/ui/theme-toggle-button.tsx (1)
14-25: Optional cleanup: Remove unnecessary template literal.The className uses a template literal but contains only a static string with no interpolation.
♻️ Simplify className
<button onClick={toggleTheme} title={dark ? "Switch to light mode" : "Switch to dark mode"} - className={` border-2 outline-0 rounded-full border-zinc-300/80 bg-slate-50 dark:border-zinc-600 dark:bg-slate-800 p-1.5`} + className="border-2 outline-0 rounded-full border-zinc-300/80 bg-slate-50 dark:border-zinc-600 dark:bg-slate-800 p-1.5" >components/footer.tsx (1)
27-27: Minor cleanup: Remove trailing space in className.Line 27 has an extra space at the end of the className string.
♻️ Remove trailing space
- <footer className="border-t border-gradient-to-r from-[#228B22]/20 to-[#FFBF00]/20 bg-gradient-to-r from-[#228B22]/5 via-[#91A511]/5 to-[#FFBF00]/5 backdrop-blur-sm mt-20 "> + <footer className="border-t border-gradient-to-r from-[#228B22]/20 to-[#FFBF00]/20 bg-gradient-to-r from-[#228B22]/5 via-[#91A511]/5 to-[#FFBF00]/5 backdrop-blur-sm mt-20">
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/page.tsxapp/submit/page.tsxcomponents/footer.tsxcomponents/pagination.tsxcomponents/ui/theme-toggle-button.tsx
🔇 Additional comments (8)
app/page.tsx (2)
10-10: LGTM: ThemeToggleButton integration.The import and placement of ThemeToggleButton in the header are correct and consistent with the submit page implementation.
Also applies to: 112-112
102-102: LGTM: Dark mode styling.The dark mode gradient classes provide appropriate contrast for the page background in dark theme.
components/footer.tsx (1)
34-34: LGTM: Dark mode text styling.The dark mode text color classes (dark:text-gray-300) provide appropriate contrast for the footer in dark theme.
Also applies to: 38-38
app/submit/page.tsx (3)
4-4: LGTM: ThemeToggleButton integration.The import and placement of ThemeToggleButton in the header match the implementation in the home page.
Also applies to: 25-25
14-16: LGTM: Consistent dark mode styling.The root container and header dark mode classes align with the styling pattern used throughout the application.
38-601: LGTM: Comprehensive dark mode styling.The dark mode text colors, backgrounds, and inline code styling are consistently applied throughout the page and provide appropriate contrast.
components/pagination.tsx (2)
50-53: LGTM: Client-side pagination dark mode styling.The dark mode classes for the Previous, Next, and page number buttons in the client-side pagination branch are correctly applied with appropriate contrast.
Also applies to: 65-68, 79-82
139-139: LGTM: Server-side Next button dark mode styling.The dark mode background class for the server-side Next link is correctly applied.
| <button | ||
| disabled | ||
| className="flex items-center gap-1 px-4 py-2 transition-colors shadow-sm bg-gray-100 text-gray-400 cursor-not-allowed" | ||
| className="flex items-center gap-1 px-4 py-2 transition-colors shadow-sm bg-gray-100 dark:bg-gray-900text-gray-400 cursor-not-allowed" |
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.
Critical: Missing space between CSS classes.
Line 147 has a syntax error where two classes are concatenated without a space: dark:bg-gray-900text-gray-400. This will cause the classes to not be applied correctly.
🐛 Fix the missing space
<button
disabled
- className="flex items-center gap-1 px-4 py-2 transition-colors shadow-sm bg-gray-100 dark:bg-gray-900text-gray-400 cursor-not-allowed"
+ className="flex items-center gap-1 px-4 py-2 transition-colors shadow-sm bg-gray-100 dark:bg-gray-900 text-gray-400 cursor-not-allowed"
>📝 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.
| className="flex items-center gap-1 px-4 py-2 transition-colors shadow-sm bg-gray-100 dark:bg-gray-900text-gray-400 cursor-not-allowed" | |
| <button | |
| disabled | |
| className="flex items-center gap-1 px-4 py-2 transition-colors shadow-sm bg-gray-100 dark:bg-gray-900 text-gray-400 cursor-not-allowed" | |
| > |
🤖 Prompt for AI Agents
In @components/pagination.tsx at line 147, The className string in the
pagination component contains a concatenated class token
("dark:bg-gray-900text-gray-400") causing a CSS syntax error; edit the className
(the disabled control or button element where className is set in
components/pagination.tsx) and insert a space between "dark:bg-gray-900" and
"text-gray-400" so the two Tailwind classes are separate and apply correctly.
| const [dark, setDark] = useState<boolean>(true) | ||
| function toggleTheme() { | ||
| setDark(pre => !pre) | ||
| document.querySelector('html')?.classList.toggle('dark') | ||
| } |
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.
Critical: Potential hydration mismatch and missing state persistence.
The theme state is hardcoded to true without checking the actual DOM state on mount. This creates several issues:
- Hydration mismatch: The initial state may not match the server-rendered HTML, causing React hydration errors in Next.js
- Lost state on reload: Theme preference doesn't persist across page refreshes
- Incorrect initial render: If a user previously selected light mode, the component will incorrectly show dark mode icons on first render
🔧 Recommended fix
"use client"
import { MoonIcon, SunIcon } from "lucide-react"
-import { useState } from "react"
+import { useState, useEffect } from "react"
function ThemeToggleButton() {
- const [dark, setDark] = useState<boolean>(true)
+ const [dark, setDark] = useState<boolean>(false)
+ const [mounted, setMounted] = useState(false)
+
+ useEffect(() => {
+ // Check localStorage or system preference on mount
+ const savedTheme = localStorage.getItem('theme')
+ const prefersDark = window.matchMedia('(prefers-color-scheme: dark)').matches
+ const isDark = savedTheme === 'dark' || (!savedTheme && prefersDark)
+
+ setDark(isDark)
+ if (isDark) {
+ document.documentElement.classList.add('dark')
+ }
+ setMounted(true)
+ }, [])
+
function toggleTheme() {
- setDark(pre => !pre)
- document.querySelector('html')?.classList.toggle('dark')
+ const newDark = !dark
+ setDark(newDark)
+ if (newDark) {
+ document.documentElement.classList.add('dark')
+ localStorage.setItem('theme', 'dark')
+ } else {
+ document.documentElement.classList.remove('dark')
+ localStorage.setItem('theme', 'light')
+ }
}
+
+ // Prevent flash during hydration
+ if (!mounted) {
+ return (
+ <button className="border-2 outline-0 rounded-full border-zinc-300/80 bg-slate-50 dark:border-zinc-600 dark:bg-slate-800 p-1.5">
+ <div className="size-5" />
+ </button>
+ )
+ }
Description
This PR fixes an issue where the article title became invisible when hovering over article cards in dark mode.
Root Cause
On hover, the title color transitioned to a value that matched or blended into the dark background, causing it to disappear.
Solution
Before
After
Related Issue
Closes #29 – Dark Mode: Article title becomes invisible on hover inside Article Cards
Checklist
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.