Feat: Add persistent dark theme toggle with localStorage state caching#335
Conversation
|
@Siddh2024 is attempting to deploy a commit to the komalsony234-1530's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@komalharshita , kindly approve my pr request ma'am. |
|
@komalharshita please approve and merge this code ma'am. |
komalharshita
left a comment
There was a problem hiding this comment.
Good feature contribution overall — the implementation is fairly clean and the persistent theme behavior is thoughtfully handled. The use of localStorage, system preference detection, and reusable toggle behavior are all solid additions.
Before merge, a few improvements are needed:
- Theme initialization currently happens after script load, which can cause a flash of incorrect theme (FOUC) before dark mode is applied.
- The stored-theme logic permanently disables future OS theme sync once a preference exists. Please clarify/document whether this behavior is intentional.
- The toggle markup is duplicated across multiple templates, which creates maintainability overhead.
- Add accessibility state handling such as
aria-pressedso screen readers can identify the active theme state. - Please verify reduced-motion/transition behavior and ensure dark-mode accessibility contrast is fully validated across all pages/components.
Overall this is a strong contribution, but a few accessibility and maintainability concerns should be addressed before approval.
…sed, and reduced-motion)
# Conflicts: # templates/project.html
# Conflicts: # static/style.css
|
@komalharshita , ma'am you can review the changes , i have made to satisfy the requirements. |
@komalharshita , Ma'am , i have done the changes , can you review ad approve this PR ?. THanks. |
komalharshita
left a comment
There was a problem hiding this comment.
Thanks for addressing the previous review feedback.
I reviewed the updated implementation and the dark mode design looks good. The improvements around theme initialization, accessibility (aria-pressed), reusable partials, and maintainability are appreciated.
At this point, the main blocker appears to be the merge conflicts, likely because another dark mode/theme-related PR has already been merged into the project.
Please sync your branch with the latest main, resolve the merge conflicts, and verify that the dark mode behavior still works correctly after rebasing (theme toggle, persistence, accessibility states, and page-wide styling).
Once the conflicts are resolved and the updated branch is pushed, I'll do a final review and I'm happy to merge the PR if everything remains stable.
Nice work overall.
|
@komalharshita , Conflicts solved ma'am, |
komalharshita
left a comment
There was a problem hiding this comment.
only resolving the merge conflicts remain
|
@komalharshita , done maam. |
|
Okay, approved for merge! |
Summary
This Pull Request introduces a native, persistent Dark Mode toggle feature across the DevPath platform. To achieve this, core color tokens (background surfaces, card containers, tech stack badges, and typography) have been refactored into global CSS variables. A lightweight JavaScript event handler handles toggling a
data-theme="dark"attribute on the document root, and the selected preference is securely cached inlocalStorageto ensure continuity across page reloads and transitions.Related Issue
Closes #318
Visuals (Screenshots/Videos):
Local Dark Mode Previews:
Checklist [required]