fix: dark mode toggle logic #1155
Conversation
Summary by CodeRabbit
WalkthroughThis pull request refactors the dark/light mode toggle component by replacing a checkbox input wrapped in a label with a button element. The button directly triggers the Changes
Assessment against linked issues
Suggested reviewers
✨ Finishing Touches
🪧 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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/components/ModeToggle.tsx (2)
20-25: Consider optimizing dark mode state management.The current implementation toggles the dark class in both the useEffect hook (lines 12-18) and the darkModeHandler function (line 23). This creates redundant DOM operations.
const darkModeHandler = () => { setDark(!dark) const newTheme = !dark ? 'dark' : 'light' - document.body.classList.toggle('dark', !dark) localStorage.setItem('theme', newTheme) }The useEffect hook will handle the class toggling when the state changes, so there's no need to do it in the handler as well.
8-10: Consider adding system preference detection.The current implementation only checks localStorage but doesn't respect the user's system preference for dark/light mode when they first visit the site.
const [dark, setDark] = useState(() => { - return localStorage.getItem('theme') === 'dark' + const storedTheme = localStorage.getItem('theme') + if (storedTheme) { + return storedTheme === 'dark' + } + return window.matchMedia('(prefers-color-scheme: dark)').matches })This would respect the user's system preference if they haven't explicitly set a theme yet.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ModeToggle.tsx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
frontend/src/components/ModeToggle.tsx (1)
frontend/src/utils/utility.ts (1) (1)
cn(15-17)
🔇 Additional comments (4)
frontend/src/components/ModeToggle.tsx (4)
28-28: Refined layout structure.The removal of the
space-x-2class from the flex container is appropriate since the toggle is now a single button component rather than multiple elements needing spacing.
36-50: Excellent UI/UX improvements for the mode toggle.The replacement of the checkbox with a proper button element significantly improves:
- Accessibility - proper semantic HTML with aria-label
- Visual feedback - smooth transitions, hover and active states
- User experience - intuitive icon representation (showing what you'll switch to)
- Keyboard navigation - proper focus states
The layered div approach creates a polished interactive effect while maintaining clean functionality.
44-44: Improved icon logic.Inverting the icon logic (
!dark ? faMoon : faSun) is more intuitive - now showing users what they'll switch to rather than the current state. This better aligns with user expectations.
38-38: Comprehensive button styling - good practice.The button styling is comprehensive with appropriate transitions, focus states, and dark mode variants. The use of
active:scale-95provides excellent tactile feedback without being distracting.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Thanks for addressing this! Please look into the following comments when you get a chance:
| </label> | ||
| <button | ||
| onClick={darkModeHandler} | ||
| className="relative h-10 w-10 transform overflow-hidden rounded-full border border-gray-300 shadow-sm transition-all duration-200 hover:shadow-md focus:outline-none focus:ring-1 focus:ring-gray-600 active:scale-95 dark:border-gray-600" |
There was a problem hiding this comment.
Could you make it look more simple? It seems there is unnecessary border when the button is in focus. Also for the light theme the background seems inconsistent comparing to what we have for the dark theme behavior.
There was a problem hiding this comment.
Thanks for the feedback! I've made the requested changes. I have made the toggle button match with the other button such as sponser to have a consistent ui also i have applied same hover effect as in other buttons . Please review and let me know if any further modifications are needed.
|
@arkid15r I've made the requested changes. I have made the toggle button match with the other button such as sponser and star to have a consistent ui also i have applied same hover effect as in other buttons . Please review and let me know if any further modifications are needed. |
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/ModeToggle.tsx (1)
20-25: Consider simplifying the theme toggle logic to avoid redundancy.The
darkModeHandlerfunction contains potentially redundant operations. Line 23 directly toggles the 'dark' class on the document body, but this is already handled by the useEffect hook on lines 12-18 that reacts to changes in thedarkstate.const darkModeHandler = () => { setDark(!dark) const newTheme = !dark ? 'dark' : 'light' - document.body.classList.toggle('dark', !dark) localStorage.setItem('theme', newTheme) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ModeToggle.tsx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
frontend/src/components/ModeToggle.tsx (1)
frontend/src/utils/utility.ts (1) (1)
cn(15-17)
🔇 Additional comments (3)
frontend/src/components/ModeToggle.tsx (3)
36-48: Great UI improvement with consistent button styling.The button implementation is a significant improvement over the previous checkbox approach. The styling now matches other UI elements like the sponsor and star buttons as requested in the previous review. The hover effects, transitions, and focus states create a polished user experience.
43-44: Icon sizing and transitions look good.The icon now displays correctly based on the theme state (sun for dark mode, moon for light mode) and includes appropriate sizing and transition effects. The additional hover rotation effect adds a nice touch to the user experience.
28-28: Simplified container styling.Removing the
space-x-2class makes sense since there's now only a single button element within the container rather than multiple elements requiring spacing.
* fix: dark mode toggle * Updated code based on review suggestions * Update code --------- Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
2 participants
Resolves #1153
Description:
This PR improves the ModeToggle component by optimizing theme persistence and enhancing UI behavior. The changes ensure a seamless dark mode experience.
Key Changes:
ModeToggle Component:
Why This is Needed:
Testing & Validation:
✔️ Verified UI updates across different screen sizes.
✔️ Ensured
localStorageproperly persists theme selection.✔️ Checked TypeScript type safety for component props.
✔️ Tested theme switching behavior with system preferences.