Feat: Add dark mode switch #1409
Conversation
WalkthroughThe pull request introduces a Changes
Sequence DiagramsequenceDiagram
participant User
participant ThemeSwitch
participant Redux
participant Document
User->>ThemeSwitch: Click theme toggle
ThemeSwitch->>Redux: Dispatch theme change action
Redux->>ThemeSwitch: Update theme state
ThemeSwitch->>Document: Update data-theme attribute
Document->>User: Render theme change
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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: 3
🧹 Nitpick comments (5)
Client/src/Components/ThemeSwitch/index.jsx (3)
7-7: Address the TODO comment regarding MUI theme integrationThe comment suggests that the theme implementation should be migrated to use MUI's theming system. This would provide better integration with Material-UI's theme context.
Would you like me to help implement the MUI theme integration? I can provide a solution that leverages MUI's
useThemehook and theme context.
12-14: Consider system theme preferenceThe component could benefit from respecting the user's system theme preference using
prefers-color-schememedia query.Here's how you can add system theme preference support:
+const getSystemTheme = () => + window.matchMedia("(prefers-color-scheme: dark)").matches ? "dark" : "light"; useEffect(() => { document.body.setAttribute("data-theme", currentTheme); }, [currentTheme]); +useEffect(() => { + const mediaQuery = window.matchMedia("(prefers-color-scheme: dark)"); + const handleChange = (e) => { + if (!localStorage.getItem("theme")) { + setCurrentTheme(e.matches ? "dark" : "light"); + } + }; + mediaQuery.addEventListener("change", handleChange); + return () => mediaQuery.removeEventListener("change", handleChange); +}, []);
17-32: Enhance accessibility with ARIA attributesWhile basic accessibility attributes are present, we can enhance them further.
Consider adding these additional attributes:
<IconButton id="theme-toggle" title="Toggles light & dark" aria-label="auto" aria-live="polite" + aria-pressed={currentTheme === "dark"} + aria-controls="theme-toggle" onClick={toggleTheme} sx={{ width: 48, height: 48, display: "flex", alignItems: "center", justifyContent: "center", }} >Client/src/Components/ThemeSwitch/index.css (1)
41-77: Optimize animations for performanceThe animations could benefit from performance optimizations to ensure smooth transitions.
Consider these performance improvements:
@media (prefers-reduced-motion: no-preference) { .sun-and-moon > .sun { transition: transform 0.5s var(--ease-elastic-3); + will-change: transform; } .sun-and-moon > .sun-beams { transition: transform 0.5s var(--ease-elastic-4), opacity 0.5s var(--ease-3); + will-change: transform, opacity; } .sun-and-moon .moon > circle { transition: transform 0.25s var(--ease-out-5); + will-change: transform; }Client/src/Pages/Auth/Login.jsx (1)
551-553: Consider ThemeSwitch placementThe ThemeSwitch component is placed at the bottom of the login form, which might not be immediately visible to users.
Consider moving it to the top navigation area alongside the logo for better visibility and consistency with common UI patterns:
-<Stack +<Stack + direction="row" + justifyContent="space-between" alignItems="center" px={theme.spacing(12)} gap={theme.spacing(4)} > - <Logo style={{ borderRadius: theme.shape.borderRadius }} /> - <Typography sx={{ userSelect: "none" }}>BlueWave Uptime</Typography> + <Stack direction="row" alignItems="center" gap={theme.spacing(4)}> + <Logo style={{ borderRadius: theme.shape.borderRadius }} /> + <Typography sx={{ userSelect: "none" }}>BlueWave Uptime</Typography> + </Stack> + <ThemeSwitch /> </Stack>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Client/src/Components/Sidebar/index.jsx(2 hunks)Client/src/Components/ThemeSwitch/SunAndMoonIcon.jsx(1 hunks)Client/src/Components/ThemeSwitch/index.css(1 hunks)Client/src/Components/ThemeSwitch/index.jsx(1 hunks)Client/src/Pages/Auth/Login.jsx(4 hunks)Client/src/index.css(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- Client/src/index.css
- Client/src/Components/ThemeSwitch/SunAndMoonIcon.jsx
🔇 Additional comments (2)
Client/src/Components/Sidebar/index.jsx (2)
24-24: Yo, this import's looking clean! 🎯
The ThemeSwitch import is properly placed with other component imports, maintaining consistent code organization.
555-583:
Yo dawg, we need to handle the collapsed state! 🤔
The theme switch is only visible in the expanded state. Consider adding it to the collapsed menu to maintain consistency across states.
Let's check if there are any other places in the codebase handling collapsed states:
| const [currentTheme, setCurrentTheme] = useState("light"); | ||
| const toggleTheme = () => | ||
| setCurrentTheme((prevState) => (prevState === "light" ? "dark" : "light")); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider persisting theme preference
The current implementation doesn't persist the user's theme preference across sessions. Consider using localStorage to remember the user's preference.
Here's how you can modify the code to persist the theme:
-const [currentTheme, setCurrentTheme] = useState("light");
+const [currentTheme, setCurrentTheme] = useState(() =>
+ localStorage.getItem("theme") || "light"
+);
const toggleTheme = () =>
- setCurrentTheme((prevState) => (prevState === "light" ? "dark" : "light"));
+ setCurrentTheme((prevState) => {
+ const newTheme = prevState === "light" ? "dark" : "light";
+ localStorage.setItem("theme", newTheme);
+ return newTheme;
+ });📝 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.
| const [currentTheme, setCurrentTheme] = useState("light"); | |
| const toggleTheme = () => | |
| setCurrentTheme((prevState) => (prevState === "light" ? "dark" : "light")); | |
| const [currentTheme, setCurrentTheme] = useState(() => | |
| localStorage.getItem("theme") || "light" | |
| ); | |
| const toggleTheme = () => | |
| setCurrentTheme((prevState) => { | |
| const newTheme = prevState === "light" ? "dark" : "light"; | |
| localStorage.setItem("theme", newTheme); | |
| return newTheme; | |
| }); |
Client/src/Pages/Auth/Login.jsx
Outdated
| <Box textAlign="center"> | ||
| <Typography | ||
| className="forgot-p" | ||
| display="inline-block" | ||
| color={theme.palette.primary.main} | ||
| > | ||
| Forgot password? | ||
| </Typography> | ||
| <Typography | ||
| component="span" | ||
| color={theme.palette.primary.main} | ||
| ml={theme.spacing(2)} | ||
| sx={{ userSelect: "none" }} | ||
| onClick={handleNavigate} | ||
| > | ||
| Reset password | ||
| </Typography> | ||
| </Box> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve keyboard accessibility for password reset
The password reset section could benefit from better keyboard accessibility.
Consider this improvement:
<Typography
component="span"
color={theme.palette.primary.main}
ml={theme.spacing(2)}
- sx={{ userSelect: "none" }}
+ sx={{
+ userSelect: "none",
+ cursor: "pointer",
+ "&:focus-visible": {
+ outline: `2px solid ${theme.palette.primary.main}`,
+ outlineOffset: "2px",
+ }
+ }}
+ tabIndex={0}
+ role="button"
+ onKeyDown={(e) => e.key === "Enter" && handleNavigate()}
onClick={handleNavigate}
>📝 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.
| <Box textAlign="center"> | |
| <Typography | |
| className="forgot-p" | |
| display="inline-block" | |
| color={theme.palette.primary.main} | |
| > | |
| Forgot password? | |
| </Typography> | |
| <Typography | |
| component="span" | |
| color={theme.palette.primary.main} | |
| ml={theme.spacing(2)} | |
| sx={{ userSelect: "none" }} | |
| onClick={handleNavigate} | |
| > | |
| Reset password | |
| </Typography> | |
| </Box> | |
| <Box textAlign="center"> | |
| <Typography | |
| className="forgot-p" | |
| display="inline-block" | |
| color={theme.palette.primary.main} | |
| > | |
| Forgot password? | |
| </Typography> | |
| <Typography | |
| component="span" | |
| color={theme.palette.primary.main} | |
| ml={theme.spacing(2)} | |
| sx={{ | |
| userSelect: "none", | |
| cursor: "pointer", | |
| "&:focus-visible": { | |
| outline: `2px solid ${theme.palette.primary.main}`, | |
| outlineOffset: "2px", | |
| } | |
| }} | |
| tabIndex={0} | |
| role="button" | |
| onKeyDown={(e) => e.key === "Enter" && handleNavigate()} | |
| onClick={handleNavigate} | |
| > | |
| Reset password | |
| </Typography> | |
| </Box> |
| }, | ||
| }} | ||
| onClick={(event) => openPopup(event, "logout")} | ||
| <ThemeSwitch /> |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
Auto Pull Request Review from LlamaPReview
1. Overview
1.1 PR Summary
- Business value and requirements alignment: This PR adds a dark mode switch feature, enhancing user experience by providing an alternative theme for users who prefer to work in a darker environment.
- Key components modified:
ThemeSwitchcomponent,Loginpage, andSidebar. - Impact assessment: The changes are primarily UI-focused and do not directly impact core application logic or data flow. However, they do interact with the theme management system of the application.
- System dependencies and integration impacts: The changes in this PR are mostly self-contained within the UI layer. However, they do interact with the theme management system of the application.
2. Detailed Technical Analysis
2.1 Code Logic Deep-Dive
Core Logic Changes
-
Client/src/Components/Sidebar/index.jsx
- Submitted PR Code:
// ... (previous code) import ThemeSwitch from "../ThemeSwitch"; // ... (previous code) <Stack flexDirection={"row"} marginLeft={"auto"} columnGap={theme.spacing(2)} > <ThemeSwitch /> <Tooltip title="Controls" disableInteractive > <IconButton // ... (rest of the code) > <DotsVertical /> </IconButton> </Tooltip> </Stack> // ... (rest of the code)
- Analysis:
- The
ThemeSwitchcomponent is added to the sidebar, allowing users to toggle between light and dark modes. - The component is wrapped in a
Stackcomponent to align it with other elements in the sidebar. - The
Tooltipcomponent is used to provide a label for the theme switch button. - The
ThemeSwitchcomponent uses theuseThemehook from Material-UI to manage the theme, and it sets thedata-themeattribute on thebodyelement to switch between light and dark modes.
- The
- Submitted PR Code:
-
Client/src/Components/ThemeSwitch/index.jsx
- Submitted PR Code:
// ... (previous code) const ThemeSwitch = () => { // ... (previous code) return ( <IconButton id="theme-toggle" title="Toggles light & dark" aria-label="auto" aria-live="polite" onClick={toggleTheme} sx={{ width: 48, height: 48, display: "flex", alignItems: "center", justifyContent: "center", }} > <SunAndMoonIcon /> </IconButton> ); }; // ... (rest of the code)
- Analysis:
- The
ThemeSwitchcomponent uses theuseEffecthook to set thedata-themeattribute on thebodyelement when the component mounts or when thecurrentThemestate changes. - The
toggleThemefunction is called when theIconButtonis clicked, toggling thecurrentThemestate between "light" and "dark". - The
SunAndMoonIconcomponent is used to display the theme switch icon.
- The
- Submitted PR Code:
-
Client/src/Pages/Auth/Login.jsx
- Submitted PR Code:
// ... (previous code) <Box textAlign="center"> <Typography className="forgot-p" display="inline-block" color={theme.palette.primary.main} > Forgot password? </Typography> <Typography component="span" color={theme.palette.primary.main} ml={theme.spacing(2)} sx={{ userSelect: "none" }} onClick={handleNavigate} > Reset password </Typography> </Box> // ... (rest of the code)
- Analysis:
- The
ThemeSwitchcomponent is added to the login page, allowing users to toggle between light and dark modes. - The component is wrapped in a
Boxcomponent withtextAlign="center"to center the component horizontally.
- The
- Submitted PR Code:
-
Client/src/index.css
- Submitted PR Code:
// ... (previous code) @import "https://unpkg.com/open-props/easings.min.css"; // ... (rest of the code)
- Analysis:
- The
open-props/easings.min.cssfile is imported to provide custom easing functions for animations.
- The
- Submitted PR Code:
Cross-cutting Concerns
- Accessibility: The
ThemeSwitchcomponent should be tested with screen readers to ensure that it's accessible to all users. Thearia-labelattribute is used to provide a label for the button, but it's important to ensure that the label is descriptive and accurate. - Responsiveness: The UI should be tested on different screen sizes and devices to ensure that it adapts well to the different themes. The use of the
Stackcomponent andflexDirection={"row"}ensures that theThemeSwitchcomponent and theIconButtonare displayed in a single row, but it's important to ensure that the UI adapts well to different screen sizes. - SEO: The use of the
data-themeattribute should be validated to ensure that it doesn't negatively impact the application's SEO. Thedata-themeattribute is used to set the theme of the page, but it's important to ensure that the attribute is used correctly and that it doesn't negatively impact the application's SEO.
2.2 Implementation Quality
- Code organization and structure: The new
ThemeSwitchcomponent is well-organized and follows the existing component structure in the project. It has a clear separation of concerns, with the icon component (SunAndMoonIcon) and the theme management logic in the mainThemeSwitchcomponent. - Design patterns usage: The use of the
useThemehook from Material-UI to manage the theme is a good practice, as it allows the component to adapt to the current theme of the application. - Error handling approach: The
ThemeSwitchcomponent does not have any explicit error handling, as the theme management is handled by the Material-UI library. However, it's important to ensure that any errors in the theme management are handled gracefully. - Resource management: The
ThemeSwitchcomponent does not have any significant resource management concerns, as it primarily relies on the Material-UI library for its functionality.
3. Critical Findings
3.1 Potential Issues
- 🔴 Critical Issues
- Impact: The
ThemeSwitchcomponent is not tested with screen readers, which could make it inaccessible to users with visual impairments. - Recommendation: Thoroughly test the
ThemeSwitchcomponent with screen readers to ensure that it's accessible to all users. Update thearia-labelattribute to provide a more descriptive label for the button.
- Impact: The
- 🟡 Warnings
- Potential risks: The use of the
data-themeattribute to manage the theme could have implications for the application's SEO. If not used correctly, it could negatively impact the application's search engine rankings. - Suggested improvements: Validate the use of the
data-themeattribute to ensure that it doesn't negatively impact the application's SEO. Consider using a more robust approach to manage the theme, such as using a theme provider component to manage the theme at a higher level in the component tree.
- Potential risks: The use of the
3.2 Code Quality Concerns
- Maintainability aspects: The
ThemeSwitchcomponent is well-maintained and follows best practices for component organization and structure. However, it's important to ensure that the component remains maintainable as the project evolves. - Readability issues: The
ThemeSwitchcomponent is well-documented and follows best practices for code readability. However, it's important to ensure that the component remains readable as the project evolves. - Performance bottlenecks: The
ThemeSwitchcomponent does not have any significant performance bottlenecks, as it primarily relies on the Material-UI library for its functionality. However, it's important to ensure that any performance optimizations are handled gracefully.
4. Security Assessment
- Authentication/Authorization impacts: The
ThemeSwitchcomponent does not have any direct authentication or authorization impacts, as it primarily focuses on managing the theme of the application. - Data handling concerns: The
ThemeSwitchcomponent does not handle any sensitive data, as it primarily focuses on managing the theme of the application. - Input validation: The
ThemeSwitchcomponent does not have any user input, as it primarily focuses on managing the theme of the application. However, it's important to ensure that any user input in the application is validated correctly to prevent security vulnerabilities. - Security best practices: The
ThemeSwitchcomponent follows best practices for security, as it primarily relies on the Material-UI library for its functionality. However, it's important to ensure that any security best practices are followed throughout the application. - Potential security risks: The use of the
data-themeattribute to manage the theme could potentially expose sensitive information if not used correctly. However, thedata-themeattribute is used to set the theme of the page, and it does not directly expose any sensitive information. - Mitigation strategies: To mitigate any potential security risks, it's important to ensure that the
data-themeattribute is used correctly and that it does not expose any sensitive information. - Security testing requirements: It's important to thoroughly test the
ThemeSwitchcomponent to ensure that it does not introduce any security vulnerabilities. This includes testing the component with different user inputs and edge cases.
5. Testing Strategy
5.1 Test Coverage
- Unit test analysis: The
ThemeSwitchcomponent does not have any unit tests, as it primarily relies on the Material-UI library for its functionality. However, it's important to ensure that any unit tests are created to cover the component's functionality. - Integration test requirements: It's important to create integration tests to ensure that the
ThemeSwitchcomponent works correctly with other components in the application. This includes testing the component with different user inputs and edge cases. - Edge cases coverage: It's important to ensure that the
ThemeSwitchcomponent is tested with different edge cases, such as when the user has a slow internet connection or when the user is using an older browser.
5.2 Test Recommendations
Suggested Test Cases
// Example test case for the ThemeSwitch component
it('should toggle the theme when the button is clicked', () => {
const { getByTestId } = render(<ThemeSwitch />);
const themeToggleButton = getByTestId('theme-toggle');
// Initially, the theme should be set to 'light'
expect(document.body.getAttribute('data-theme')).toBe('light');
// Click the theme toggle button
userEvent.click(themeToggleButton);
// After clicking, the theme should be set to 'dark'
expect(document.body.getAttribute('data-theme')).toBe('dark');
// Click the theme toggle button again
userEvent.click(themeToggleButton);
// After clicking again, the theme should be set back to 'light'
expect(document.body.getAttribute('data-theme')).toBe('light');
});- Coverage improvements: It's important to ensure that the
ThemeSwitchcomponent is tested with different user inputs and edge cases. This includes testing the component with different screen sizes and devices, as well as testing the component with different user preferences. - Performance testing needs: It's important to ensure that the
ThemeSwitchcomponent performs well under different loads and conditions. This includes testing the component with different user inputs and edge cases, as well as testing the component with different network conditions.
6. Documentation & Maintenance
- Documentation updates needed: The
ThemeSwitchcomponent is well-documented and follows best practices for code documentation. However, it's important to ensure that the component's documentation is kept up-to-date as the project evolves. - Long-term maintenance considerations: The
ThemeSwitchcomponent is well-maintained and follows best practices for component organization and structure. However, it's important to ensure that the component remains maintainable as the project evolves. This includes ensuring that the component is tested thoroughly and that any performance optimizations are handled gracefully. - Technical debt and monitoring requirements: It's important to monitor the
ThemeSwitchcomponent to ensure that it does not introduce any technical debt. This includes ensuring that the component is tested thoroughly and that any performance optimizations are handled gracefully.
7. Deployment & Operations
- Deployment impact and strategy: The
ThemeSwitchcomponent does not have any significant deployment impacts, as it primarily focuses on managing the theme of the application. However, it's important to ensure that the component is deployed correctly and that it works as expected in different environments. - Key operational considerations: The
ThemeSwitchcomponent does not have any significant operational considerations, as it primarily focuses on managing the theme of the application. However, it's important to ensure that the component is monitored correctly and that any performance optimizations are handled gracefully.
8. Summary & Recommendations
8.1 Key Action Items
- Critical changes required: Thoroughly test the
ThemeSwitchcomponent with screen readers to ensure that it's accessible to all users. Update thearia-labelattribute to provide a more descriptive label for the button. - Important improvements suggested: Validate the use of the
data-themeattribute to ensure that it doesn't negatively impact the application's SEO. Consider using a more robust approach to manage the theme, such as using a theme provider component to manage the theme at a higher level in the component tree. - Best practices to implement: Ensure that the
ThemeSwitchcomponent is tested thoroughly and that any performance optimizations are handled gracefully. Follow best practices for code documentation and maintainability.
8.2 Future Considerations
- Technical evolution path: As the project evolves, it's important to ensure that the
ThemeSwitchcomponent remains maintainable and that it adapts well to changes in the project. This includes ensuring that the component is tested thoroughly and that any performance optimizations are handled gracefully. - Business capability evolution: As the business evolves, it's important to ensure that the
ThemeSwitchcomponent remains relevant and that it adapts well to changes in the business. This includes ensuring that the component is tested thoroughly and that any performance optimizations are handled gracefully. - System integration impacts: As the system integrates with other components, it's important to ensure that the
ThemeSwitchcomponent remains compatible and that it adapts well to changes in the system. This includes ensuring that the component is tested thoroughly and that any performance optimizations are handled gracefully.
💡 Help Shape LlamaPReview
How's this review format working for you? Vote in our Github Discussion Polls to help us improve your review experience!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Client/src/Components/ThemeSwitch/index.jsx (2)
1-6: Yo dawg, let's organize these imports!Consider grouping imports in this order:
- React and core libraries
- Third-party components (Material-UI)
- Redux-related imports
- Local components and styles
import React, { useEffect, useState } from "react"; +import { useDispatch, useSelector } from "react-redux"; import { IconButton } from "@mui/material"; +import { setMode } from "../../Features/UI/uiSlice"; import SunAndMoonIcon from "./SunAndMoonIcon"; import "./index.css"; -import { useDispatch, useSelector } from "react-redux"; -import { setMode } from "../../Features/UI/uiSlice";
12-14: Knees weak, arms heavy: Let's make this more maintainable!Consider extracting theme values into constants to prevent magic strings and make maintenance easier.
+const THEME_MODES = { + LIGHT: "light", + DARK: "dark", +} as const; + const toggleTheme = () => { - dispatch(setMode(mode === "light" ? "dark" : "light")); + dispatch(setMode(mode === THEME_MODES.LIGHT ? THEME_MODES.DARK : THEME_MODES.LIGHT)); };
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Client/src/Components/ThemeSwitch/SunAndMoonIcon.jsx(1 hunks)Client/src/Components/ThemeSwitch/index.css(1 hunks)Client/src/Components/ThemeSwitch/index.jsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Client/src/Components/ThemeSwitch/index.css
- Client/src/Components/ThemeSwitch/SunAndMoonIcon.jsx
🔇 Additional comments (3)
Client/src/Components/ThemeSwitch/index.jsx (3)
8-10: Mom's spaghetti: Let's not forget about persistence!
The current implementation doesn't persist the user's theme preference across sessions.
20-37: He's nervous, but on the surface he looks calm and ready: Great accessibility implementation!
The IconButton implementation includes all necessary accessibility attributes and clean styling. Well done! 👍
16-18: 🛠️ Refactor suggestion
There's vomit on his sweater: Add cleanup to useEffect!
Consider adding a cleanup function to reset the theme attribute when component unmounts.
useEffect(() => {
document.body.setAttribute("data-theme", mode);
+ return () => {
+ document.body.removeAttribute("data-theme");
+ };
}, [mode]);Likely invalid or redundant comment.
There was a problem hiding this comment.
@peterpardo The functionality looks good to me!
I like the UI and UX of the button here, however I think the implementatino is a bit heavy for a simple toggle button.
The other issue is that the code used is distributed under the Apache license and proper attribution hasn't been made.
Why don't we drop the licensed code and roll our own implementation for the toggle here?
The fewer licenses we have to juggle the better 👍
I think Apache license is more than fine. If we can make an attribution to the Apache license, we are good to go here. I don't have technical expertise to comment on the code/implementation tho :) |
@gorkem-bwl the implementation seems a bit excessive for a simple toggle to me, but it does look nice 😂 If you're OK with the license and like the animation then I will defer. @peterpardo please include the required attributions in the comments then and we can merge after @marcelluscaio and @shemy-gan sign off 👍 |
🫡 |
Do we also add attributions in the For now, I added a comment at the top of the |
@peterpardo I think the attribution in the comments is sufficient here 👍 We can merge this as soon as the front end team has a once over. |
There was a problem hiding this comment.
The implementation was good, but I suggest we make some changes to it to avoid two different logics for the feature.
Also, I would ask you to try a different design approach. Try to place the icon on the top right corner (respecting the page's padding).
One other thing: we should delete the current theme toggle that lives in the settings.
Thanks again for your contribution!
| useEffect(() => { | ||
| document.body.setAttribute("data-theme", mode); | ||
| }, [mode]); |
There was a problem hiding this comment.
I understand the logic from this implementation relies on data-attributes. But we already have a theme on redux. I suggest adapting that to rely on our theme as well. This way you can ditch this useEffect
There was a problem hiding this comment.
Got it. Let me update the component.
There was a problem hiding this comment.
Here is the commit for the changes I made: 7c5c350
Client/src/index.css
Outdated
| @@ -1,4 +1,5 @@ | |||
| @import url("https://fonts.googleapis.com/css2?family=Inter:ital,opsz,wght@0,14..32,100..900;1,14..32,100..900&display=swap"); | |||
| @import "https://unpkg.com/open-props/easings.min.css"; | |||
There was a problem hiding this comment.
I think this one is responsible for importing the easing functions that is being used by the toggle switch. I found it here in the code of the css file https://web.dev/patterns/theming/theme-switch.
This are currently the easing variables that is being used:
--ease-elastic-3--ease-elastic-4--ease-out-5
There was a problem hiding this comment.
@peterpardo we don't want the project to depened on a CDN that we can't guarantee is going to be available at all times.
Can you please copy only the css that is needed from that css file and add it locally? We can drop the import then and ensure the code wil be working at all times.
There was a problem hiding this comment.
Got it. I'll check how they did the animation.
There was a problem hiding this comment.
Here are the new changes. I've removed the import and instead just took the code needed for the smooth animation of the toggle to work.
In fact I didn't want to add it there since we don't have a proper horizontal area to add it when the page starts with a setting block. If Peter adds it on top right corner all the contents will be pushed down. How about we defter moving it to up right position, and see if we can add any other features (eg a global search) in that area together with the theme switcher? 
|
|
@gorkem-bwl I am not sure about that. We do have a sidebar. What if that toggle came before the menu items? After the logo. And we could have it positioned in the top left in the login page. I didn't really like it in the middle |
Adding it to the sidebar's top right corner is ok for me - When you said "top right" I thought it would be the whole page's "top right position" which will push down all the right hand side content. The toggle in the middle is to preserve symmetry of the wholep page. Is there a reason you don't like it there? |
Client/src/index.css
Outdated
| --ease-elastic-3: cubic-bezier(0.68, -0.55, 0.27, 1.55); | ||
| --ease-elastic-4: cubic-bezier(0.68, -0.55, 0.27, 1.55); | ||
| --ease-3: cubic-bezier(0.25, 0.1, 0.25, 1); | ||
| --ease-out-5: cubic-bezier(0.4, 0, 0.2, 1); |
There was a problem hiding this comment.
@peterpardo we aren't going to use these anywhere else in the application, can you move them to the ThemeSwitch specific CSS file? Thanks
There was a problem hiding this comment.
Ok, let me work on this real quick.
There was a problem hiding this comment.
Here is the commit for this change: b97e70f.
I directly used the styling inside the ThemeSwitch css file since it will only be used there. Thanks!
|
@peterpardo Pleaes also note that |
|
@peterpardo you don't need to worry about the position of the toggle as I had mentioned before. After solving the comments on the merging I think we are ready |
| <IconButton | ||
| id="theme-toggle" | ||
| title="Toggles light & dark" | ||
| className={`theme-${mode}`} |
There was a problem hiding this comment.
I really liked this approach =)
There was a problem hiding this comment.
Awesome! Thank you. I really appreciate that.
Got it. Forgot to update my branch with the latest |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
Client/src/Pages/Auth/Login/Login.jsx (1)
179-179: Spacing game on point! 👌The rowGap addition provides clean vertical separation. Consider making it responsive for different screen sizes.
-rowGap={theme.spacing(8)} +rowGap={{ xs: theme.spacing(6), sm: theme.spacing(8) }}Client/src/Pages/Auth/Login/Components/ForgotPasswordLabel.jsx (1)
1-8: Yo dawg, consider using@mui/material/stylesfor theme consistency!The import of
useThemefrom@mui/materialmight not pick up custom theme modifications. For better dark mode support, consider importing from@mui/material/stylesinstead.-import { Box, Typography, useTheme } from "@mui/material"; +import { Box, Typography } from "@mui/material"; +import { useTheme } from "@mui/material/styles";Client/src/Pages/Auth/Login/Components/PasswordStep.jsx (1)
125-125: Yo, while we're adding dark mode, let's make those styles pop!The component uses theme-aware styling, but we could enhance the dark mode experience by adjusting the contrast and transitions:
const PasswordStep = ({ form, errors, onSubmit, onChange, onBack }) => { const theme = useTheme(); + const mode = theme.palette.mode; + + const getTransitionStyles = () => ({ + transition: theme.transitions.create(['background-color', 'color'], { + duration: theme.transitions.duration.standard, + }), + backgroundColor: mode === 'dark' ? theme.palette.background.paper : theme.palette.background.default, + }); // Add the transition styles to your Stack component's sx prop return ( <Stack sx={{ + ...getTransitionStyles(), // existing styles... }} >
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Client/src/Components/Sidebar/index.jsx(2 hunks)Client/src/Components/ThemeSwitch/index.css(1 hunks)Client/src/Pages/Auth/Login/Components/EmailStep.jsx(0 hunks)Client/src/Pages/Auth/Login/Components/ForgotPasswordLabel.jsx(1 hunks)Client/src/Pages/Auth/Login/Components/PasswordStep.jsx(1 hunks)Client/src/Pages/Auth/Login/Login.jsx(3 hunks)
💤 Files with no reviewable changes (1)
- Client/src/Pages/Auth/Login/Components/EmailStep.jsx
🚧 Files skipped from review as they are similar to previous changes (2)
- Client/src/Components/Sidebar/index.jsx
- Client/src/Components/ThemeSwitch/index.css
🔇 Additional comments (2)
Client/src/Pages/Auth/Login/Login.jsx (2)
16-17: Yo, these imports are straight fire! 🔥
The new component imports are properly organized and aligned with the existing import structure.
216-216: Let's make sure this theme switch is connected right! 🔍
Verify that the ThemeSwitch component properly integrates with the existing theme system.
✅ Verification successful
Yo, the theme switch is perfectly wired up! 🎯
The implementation is solid with a proper Redux-based theme management system:
- ThemeSwitch component dispatches theme changes through Redux
- App.jsx properly applies the theme using MUI's ThemeProvider
- Complete theme configurations (light/dark) are well-defined with proper color palettes
- Theme changes affect the entire app through global styles and CSS baseline
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for theme-related imports and usage
rg -l "useTheme|ThemeProvider|createTheme" --type jsx
Length of output: 83
Script:
#!/bin/bash
# Let's try different file extensions and check theme-related patterns
rg -l "useTheme|ThemeProvider|createTheme"
# Also check for ThemeSwitch component definition and usage
rg -l "ThemeSwitch"
# Look for any theme-related configuration files
fd -e js -e jsx -e ts -e tsx "theme"
# Check for MUI theme imports specifically
rg -l "@mui.*theme"
Length of output: 4479
Script:
#!/bin/bash
# Let's check the ThemeSwitch component implementation
cat Client/src/Components/ThemeSwitch/index.jsx
# Check the theme configuration files
cat Client/src/Utils/Theme/globalTheme.js
cat Client/src/Utils/Theme/darkTheme.js
cat Client/src/Utils/Theme/lightTheme.js
# Check how theme state is managed in App.jsx
cat Client/src/App.jsx
Length of output: 14726
| <ForgotPasswordLabel | ||
| email={form.email} | ||
| errorEmail={errors.email} | ||
| /> | ||
| <Box marginX={"auto"}> | ||
| <ThemeSwitch /> | ||
| </Box> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hold up, let's talk about that layout! 🤔
The components are properly integrated, but the ThemeSwitch placement at the bottom might not be the most intuitive location for users.
Consider moving the ThemeSwitch to the top header area for better visibility and accessibility:
<Stack
direction="row"
alignItems="center"
px={theme.spacing(12)}
gap={theme.spacing(4)}
>
<Logo style={{ borderRadius: theme.shape.borderRadius }} />
<Typography sx={{ userSelect: "none" }}>BlueWave Uptime</Typography>
+ <Box marginLeft="auto">
+ <ThemeSwitch />
+ </Box>
</Stack>This would:
- Make the theme switch more discoverable
- Follow common UI patterns where theme toggles are in the header
- Free up space in the main content area
Committable suggestion skipped: line range outside the PR's diff.
| ForgotPasswordLabel.proptype = { | ||
| email: PropTypes.string.isRequired, | ||
| emailError: PropTypes.string.isRequired, | ||
| }; |
There was a problem hiding this comment.
Yo, we got some PropTypes issues that's making me nervous!
There are two critical issues in the PropTypes declaration:
proptypeis misspelled (should bepropTypes)emailErrorprop type doesn't match theerrorEmailprop used in the component
-ForgotPasswordLabel.proptype = {
+ForgotPasswordLabel.propTypes = {
email: PropTypes.string.isRequired,
- emailError: PropTypes.string.isRequired,
+ errorEmail: PropTypes.string.isRequired,
};📝 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.
| ForgotPasswordLabel.proptype = { | |
| email: PropTypes.string.isRequired, | |
| emailError: PropTypes.string.isRequired, | |
| }; | |
| ForgotPasswordLabel.propTypes = { | |
| email: PropTypes.string.isRequired, | |
| errorEmail: PropTypes.string.isRequired, | |
| }; |
| <Typography | ||
| component="span" | ||
| color={theme.palette.primary.main} | ||
| ml={theme.spacing(2)} | ||
| sx={{ userSelect: "none" }} | ||
| onClick={handleNavigate} | ||
| > | ||
| Reset password | ||
| </Typography> | ||
| </Box> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Yo, let's make this reset password link more accessible!
The reset password text should be more interactive and accessible:
- Missing hover state for better UX
- Should be a button or link for keyboard navigation
- Needs cursor pointer style
<Typography
- component="span"
+ component="button"
color={theme.palette.primary.main}
ml={theme.spacing(2)}
- sx={{ userSelect: "none" }}
+ sx={{
+ userSelect: "none",
+ cursor: "pointer",
+ background: "none",
+ border: "none",
+ padding: 0,
+ '&:hover': {
+ textDecoration: 'underline'
+ }
+ }}
onClick={handleNavigate}
>
Reset password
</Typography>📝 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.
| <Typography | |
| component="span" | |
| color={theme.palette.primary.main} | |
| ml={theme.spacing(2)} | |
| sx={{ userSelect: "none" }} | |
| onClick={handleNavigate} | |
| > | |
| Reset password | |
| </Typography> | |
| </Box> | |
| <Typography | |
| component="button" | |
| color={theme.palette.primary.main} | |
| ml={theme.spacing(2)} | |
| sx={{ | |
| userSelect: "none", | |
| cursor: "pointer", | |
| background: "none", | |
| border: "none", | |
| padding: 0, | |
| '&:hover': { | |
| textDecoration: 'underline' | |
| } | |
| }} | |
| onClick={handleNavigate} | |
| > | |
| Reset password | |
| </Typography> | |
| </Box> |
|
I've extracted the Forgot password label from the Let me know what do you think. |
Here is the commit for this change: b5a125b Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
Client/src/Pages/Auth/Login/Components/PasswordStep.jsx (1)
Line range hint
94-109: Knees weak, arms are heavy, that fixed width's making me nervous! 💪The LoadingButton's fixed width of 30% might not work well on all screen sizes.
Let's make it more responsive:
<LoadingButton variant="contained" color="primary" type="submit" loading={authState.isLoading} disabled={errors.password && true} sx={{ - width: "30%", + minWidth: "120px", + width: { xs: "100%", sm: "30%" }, "&.Mui-focusVisible": { outline: `2px solid ${theme.palette.primary.main}`, outlineOffset: `2px`, boxShadow: `none`, }, }} >
🧹 Nitpick comments (2)
Client/src/Pages/Auth/Login/Components/PasswordStep.jsx (2)
Line range hint
28-32: Mom's spaghetti says we need better keyboard navigation! 🍝The focus management is good, but we could enhance the keyboard navigation experience.
Consider adding keyboard shortcuts for common actions:
useEffect(() => { if (inputRef.current) { inputRef.current.focus(); } + const handleKeyDown = (e) => { + if (e.key === 'Escape') { + onBack(); + } + }; + document.addEventListener('keydown', handleKeyDown); + return () => document.removeEventListener('keydown', handleKeyDown); }, []);
124-124: There's semicolon on his export already! 😅The added semicolon at line 124 is unnecessary in JSX files and inconsistent with the rest of the codebase.
-export default PasswordStep; +export default PasswordStep
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Client/src/Pages/Auth/Login/Components/EmailStep.jsx(0 hunks)Client/src/Pages/Auth/Login/Components/PasswordStep.jsx(1 hunks)
💤 Files with no reviewable changes (1)
- Client/src/Pages/Auth/Login/Components/EmailStep.jsx
🔇 Additional comments (1)
Client/src/Pages/Auth/Login/Components/PasswordStep.jsx (1)
Line range hint 1-10: Yo dawg, let's make sure our theme game is strong! 🎵
The component uses theme properly, but with the new dark mode switch being added, we should verify the contrast ratios for both themes.
Let's check if we're handling both themes correctly:
✅ Verification successful
Yo, the theme implementation is clean like mom's spaghetti! 🍝
After checking the code, I'm dropping these bars to say we're all good! The component:
- Uses
useThemehook properly - No hardcoded colours in sight, fam
- All styling uses theme tokens (spacing and palette)
- Theme properties are properly referenced for both light and dark modes through
theme.palette
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any hardcoded colors that might not respect the theme
rg -g '*.{jsx,js}' --no-heading '(#[0-9A-Fa-f]{6}|#[0-9A-Fa-f]{3}|rgb[a]?\(|hsl[a]?\()' ./Client/src/Pages/Auth/Login/Components/
Length of output: 131
Script:
#!/bin/bash
# Let's check the theme usage in the component
ast-grep --pattern 'useTheme()' ./Client/src/Pages/Auth/Login/Components/PasswordStep.jsx
# Let's also check if we're using theme properties correctly
rg -g '*.{jsx,js}' --no-heading 'theme\.' ./Client/src/Pages/Auth/Login/Components/PasswordStep.jsx
# Check for any style-related props or components that might need theme consideration
ast-grep --pattern 'sx={$_}'
Length of output: 640
There was a problem hiding this comment.
@peterpardo nice work, thank you for working through revisions with us. Looks good to me!
Summary
This PR adds a dark mode switch or theme switch button that will be used to toggle dark mode.
Changes Made
ThemeSwitchcomponentLoginpage and theSidebarto use theThemeSwitchThemeSwitchbutton functionality workingRelated Issue/s
Fixes #1317
Screenshots / Demo Video
Login Page
https://github.com/user-attachments/assets/d20f1083-0dd3-4505-9b9b-b949ae38140c
Side bar
https://github.com/user-attachments/assets/d4346b69-13d7-4ecd-8cc9-2ae97082109c
Additional Context
Used this as reference https://web.dev/patterns/theming/theme-switch