feat:Add Light/Dark Theme Toggle Using Redux - #8
Open
SaumyaDwivedi179 wants to merge 4 commits into
Open
SaumyaDwivedi179 wants to merge 4 commits into
SaumyaDwivedi179 wants to merge 4 commits into
Conversation
Collaborator
|
@cw-pr-agent review |
1 similar comment
Collaborator
|
@cw-pr-agent review |
Change SummaryImplements Redux-driven light/dark theming for the Weather App. File Changes
Based on 16e6dac...9ca410c |
PR ScorecardScoreScoring MethodologyCommunication Scoring FrameworkThe overall communication score is a weighted average:
Formula: Code Scoring FrameworkThe scorecard evaluates code using 3 key reviewer questions:
PR Communication NotesDescription Quality
PR Size & Scope
Commit Messages
Issue NotesCode Correctness & Design Quality
Test Quality & Coverage
Code Readability & Maintainability
💬 Minor Issues (Nitpicks)Code Readability & Maintainability
Based on 16e6dac...7445134 |
![mergemitra[bot]](https://avatars.githubusercontent.com/in/2493799?s=60&v=4){kind=small}
| Get instant weather updates for any city worldwide | ||
| </p> | ||
| </div> | ||
| <ThemeToggle /> {} |
There was a problem hiding this comment.
Severity: 🔴 Critical
Remove the stray {} after <ThemeToggle /> to avoid a JSX parse error that will break the build.
| theme: localStorage.getItem('theme') === 'dark' ? 'dark' : 'light' | ||
| }, | ||
| reducers: { | ||
| toggleTheme: (state) => { |
There was a problem hiding this comment.
Severity: 🔴 Critical
Keep reducers pure by removing localStorage.setItem from toggleTheme; persist the theme in an effect/middleware with try/catch.
toggleTheme: (state) => {
state.theme = state.theme === 'light' ? 'dark' : 'light'
}
| const dispatch = useDispatch() | ||
|
|
||
| useEffect(() => { | ||
| document.documentElement.className = theme |
There was a problem hiding this comment.
Severity: 🟠 Major
Avoid overwriting all <html> classes; toggle only the theme classes (and remove the unused dispatch/toggleTheme import).
useEffect(() => {
document.documentElement.classList.toggle('dark', theme === 'dark')
document.documentElement.classList.toggle('light', theme === 'light')
}, [theme])
PR OverviewPR Type: Feature Focus Areas for Architect Review
PR InsightsPotential PR Improvements
PR Strengths
|
|
Tip Need another review? Tag me and say rereview for re-analysis after you have fixed all the issues. @cw-pr-agent rereview |
Collaborator
|
@cw-pr-agent rereview |
PR ScorecardScoreScoring MethodologyCommunication Scoring FrameworkThe overall communication score is a weighted average:
Formula: Code Scoring FrameworkThe scorecard evaluates code using 3 key reviewer questions:
PR Communication NotesDescription Quality
PR Size & Scope
Commit Messages
Issue NotesCode Correctness & Design Quality
Test Quality & Coverage
Code Readability & Maintainability
Based on 7445134...9ca410c |
|
|
||
| export function ThemeProvider({ children }) { | ||
| const theme = useSelector((state) => state.theme.theme) | ||
| const dispatch = useDispatch() |
There was a problem hiding this comment.
Severity: 🟠 Major
dispatch is unused here; remove useDispatch() (and its import) or actually use it, and if theme persistence is still desired, persist in this effect (not in the reducer) with a guarded localStorage write.
useEffect(() => {
...
try { localStorage.setItem('theme', theme) } catch {}
}, [theme])
| theme: localStorage.getItem('theme') === 'dark' ? 'dark' : 'light' | ||
| }, | ||
| reducers: { | ||
| toggleTheme: (state) => { |
There was a problem hiding this comment.
Severity: 🟠 Major
If the slice still initializes from localStorage, make that read resilient (storage can throw in private mode/blocked contexts) and keep the reducer pure.
const initialTheme = (() => { try { return localStorage.getItem('theme') ?? 'light' } catch { return 'light' } })()
| @@ -0,0 +1,17 @@ | |||
|
|
|||
There was a problem hiding this comment.
Severity: 🟠 Major
Add automated tests for toggleTheme (and persistence/DOM-class syncing if kept) so regressions are caught by CI—at minimum a slice unit test and a ThemeProvider effect smoke test.
PR OverviewPR Type: Feature Focus Areas for Architect Review
Rereview ImpressionsProgress Since Last Review
Remaining Concerns
New Issues Introduced (if any)
PR InsightsPotential PR Improvements
PR Strengths
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What This PR Does
themeSlicewithtoggleThemeaction andthemeReducerinsrc/store/themeSlice.js.src/store/store.jsand wraps the app with<Provider>inMain.jsx.AppContext.jsxor header component) to switch between light and dark themes usinguseDispatchanduseSelector.Manual Testing Steps (Reviewer)
npm install&npm run devto start the app.Files Changed
src/store/themeSlice.js→ AddedthemeSlicewithtoggleThemeaction.src/store/store.js→ Added Redux store configuration.src/components/AppContext.jsx→ Added toggle button UI usinguseDispatch&useSelector.src/Main.jsx→ Wrapped with<Provider>& applied theme styles based on Redux state.src/index.css→ Added CSS variables for light/dark themes.✅ Definition of Done
Code Quality
Testing