feat: add dark mode toggle with localStorage persistence#1109
feat: add dark mode toggle with localStorage persistence#1109karkera-saakshi wants to merge 4 commits into
Conversation
|
Thank you @, for creating the PR and contributing to our UltimateHealth project 💗. |
bc05a41 to
2eb9693
Compare
|
/review |
🤖 Gemini AI Code ReviewSummaryThis Pull Request introduces dark mode functionality to the application using While the intention and initial setup are good, the implementation suffers from several critical issues related to maintainability, potential regressions, and styling best practices, primarily an over-reliance on 🔴 High Severity
🟡 Medium Severity
🟢 Low Severity / Nits
What's Good ✅
VerdictRequest Changes The PR introduces several high-severity issues, most notably the loss of critical SEO/Open Graph metadata, a potential regression for base path-aware links, and a pervasive over-reliance on |
SB2318
left a comment
There was a problem hiding this comment.
🔴 High Severity
SEO Metadata Regression
The PR removes existing
keywordsandopenGraphmetadata fromlayout.tsxand replaces the detailed description with a generic placeholder.This is a significant SEO and social-sharing regression that appears unrelated to the dark mode implementation.
Please restore the original metadata unless there is a documented reason for changing it.
Base Path Compatibility Regression
The "Join Us & Contribute" link has been changed from:
href={withBasePath("/contribute")}to:
href="/contribute"This may break deployments hosted under a custom base path. Please keep
withBasePath()usage to ensure compatibility across deployment environments.
🟡 Medium Severity
Inconsistent Theme Variable Naming
Inline Style Added to Hero Section
The following inline style was introduced:
style={{ paddingTop: "80px" }}Inline styles make maintenance and theme customization more difficult.
Please move this into a CSS class or Tailwind utility class.
Mixed Styling Approaches
feature-cardnow combines:
- Custom CSS definitions
- Tailwind hover classes
Styling for the same component is now split across multiple locations, making future maintenance harder.
Consider keeping hover behavior in a single place for consistency.
🟢 Low Severity
Remove Commented-Out Code
theme-provider.tsxcontains an entire commented implementation above the active implementation.Please remove unused commented code to keep the file clean.
✅ Positive Feedback
Nice work on the overall dark mode setup:
- Proper integration of
next-themes- Correct use of
ThemeProvidersuppressHydrationWarningadded to prevent hydration mismatches- Theme toggle implementation follows recommended patterns
- System theme support is configured correctly
The dark mode foundation looks solid. The primary concerns are around maintainability, SEO regressions, and styling architecture that should be addressed before merging.
Verdict
Request Changes
The dark mode feature itself is a valuable addition, but the SEO metadata removal, base path regression risk, and extensive use of !important introduce maintainability and production concerns that should be resolved before merge.
81a102f to
0db4694
Compare
|
Hi @SB2318, I have addressed all the high severity issues: |
0db4694 to
945296b
Compare
|
You modified If you want to support dark mode, there's no need to rewrite global styles or update every component's CSS. Tailwind already provides built-in dark mode utilities. You can simply add classes like: className="bg-white dark:bg-black"or className="text-black dark:text-white"and apply them only where needed. This keeps dark mode changes component-scoped and avoids introducing global styling changes that impact the entire UI. Tailwind's recommended approach is to use the Reference: https://tailwindcss.com/docs/dark-mode |
|
@karkera-saakshi please consider @rushiii3 's Point, and please update. I am available. |
|
Hi @SB2318 I am available and working on it. |
PR Description
Closes #930 Add Dark Mode Support for Better Visibility and User Experience
What's Changed
Testing
Type of Change
Select your work-area
Related Issue
Please provide a link to the issue solved if applicable.
Add your Work Example
Showcasing light and dark mode
darkmode.1.mp4
Reloading the page while in dark or light mode
reloading.mp4
Opening the same web in new tab to see it still ahs the same setting for modes as before
new.tab.mp4
Fixes (mention the issue number which this fixes)
#930
Checklist
Undertaking
My code follows the style guidelines of this project.
I have performed a self-review of my code.
I have commented my code, particularly in hard-to-understand areas.
I have made corresponding changes to the documentation.
I have checked for plagiarism and assure its authenticity.
I have read and followed the code of conduct for this repository. I understand that violation of this undertaking may have legal consequences.
I Agree