Update Themetoggle and Improve Footer Text Visibility #422
Conversation
|
@AnushkaChouhan25 is attempting to deploy a commit to the bunty's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for submitting your pull request! 🙌 We'll review it as soon as possible. In the meantime, please ensure that your changes align with our CONTRIBUTING.md. If there are any specific instructions or feedback regarding your PR, we'll provide them here. Thanks again for your contribution! 😊 |
WalkthroughThe changes in this pull request involve updates to three main files: Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (5)
frontend/src/index.css (1)
Line range hint
27-30: Remove commented code.Consider removing the commented-out scrollbar styles if they're no longer needed. This will improve code maintainability.
frontend/src/components/ThemeToggle.jsx (2)
19-20: Use CSS variable for consistent transition duration.The component uses a hardcoded duration of 300ms while there's a CSS variable
--transition-durationset to 500ms. For consistency, consider using the CSS variable.-<span className={`block w-16 h-8 bg-gray-300 rounded-full shadow-inner transition duration-300 ease-in-out ${isDarkMode ? 'bg-gray-800' : 'bg-gray-200'}`}></span> +<span className={`block w-16 h-8 bg-gray-300 rounded-full shadow-inner transition duration-[var(--transition-duration)] ease-in-out ${isDarkMode ? 'bg-gray-800' : 'bg-gray-200'}`}></span>
36-39: Use CSS variable for slider transition.For consistency with other animations, use the
--transition-durationCSS variable.-className={`absolute w-7 h-7 bg-white rounded-full shadow-lg transition-transform duration-300 ease-in-out transform ${isDarkMode ? 'translate-x-8' : 'translate-x-0'}`} +className={`absolute w-7 h-7 bg-white rounded-full shadow-lg transition-transform duration-[var(--transition-duration)] ease-in-out transform ${isDarkMode ? 'translate-x-8' : 'translate-x-0'}`}frontend/src/components/Shared/footer/Content.jsx (2)
Line range hint
19-58: Consider improving the NewsletterForm positioningThe current absolute positioning (
absolute top-12 right-60) of the newsletter form could cause layout issues across different screen sizes. Consider using a more flexible layout approach:- <div className="absolute top-12 right-60 p-2 rounded-md shadow-lg "> + <div className="relative p-2 rounded-md shadow-lg">Also, consider enhancing the email validation:
<input type="email" className="p-2 rounded-r-none border-l border-gray-300 focus:outline-none" placeholder="Enter your email" value={email} onChange={(e) => setEmail(e.target.value)} + pattern="[a-z0-9._%+-]+@[a-z0-9.-]+\.[a-z]{2,}$" + title="Please enter a valid email address" required />
Line range hint
186-192: Remove commented codeThe commented-out Google Translate image section should be removed since it's been replaced with the
<Google />component. This will improve code maintainability.- {/* <div className="flex items-center justify-center mt-4"> - <img - src={googleImage} - alt="Google Translate" - className="w-[2rem] h-[2rem] mr-[65px]" - /> - </div> */}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- frontend/src/components/Shared/footer/Content.jsx (1 hunks)
- frontend/src/components/ThemeToggle.jsx (1 hunks)
- frontend/src/index.css (1 hunks)
🔇 Additional comments (4)
frontend/src/index.css (1)
Line range hint
1-53: Verify footer text visibility in light theme.While dark mode styles have been added, I don't see specific styles addressing the footer text visibility in light theme, which was one of the PR objectives.
#!/bin/bash # Search for footer-related styles rg -i "footer|foot" --type cssfrontend/src/components/ThemeToggle.jsx (2)
10-17: LGTM! Follows accessibility best practices.The implementation uses proper semantic HTML with accessible form controls and screen reader support.
41-44: Verify the need for duplicate icon implementation.The code renders the same icons twice:
- First set at lines 23-34 (moon/sun icons for different modes)
- Second set within the circular container (lines 41-44)
This duplication might be unnecessary and could cause confusion. If both sets are intentional for the design, consider:
- Adding comments explaining the purpose of each set
- Using CSS to hide one set when the other is visible
frontend/src/components/Shared/footer/Content.jsx (1)
92-94: Good improvement in text visibility!The new text color (#004D43) provides better contrast against the light theme background while maintaining dark mode compatibility. This change effectively addresses the footer text visibility issue mentioned in the PR objectives.
| /* Enable dark mode */ | ||
| html.dark { | ||
| background-color: #1a202c; /* Dark slate background */ | ||
| color: #f0e7db; /* Light text */ | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add transitions for smooth theme switching.
The dark mode colors provide excellent contrast (14:1 ratio) for accessibility. However, consider adding transitions for a smoother theme switch experience.
html.dark {
+ transition: background-color var(--transition-duration) ease-in-out,
+ color var(--transition-duration) ease-in-out;
background-color: #1a202c; /* Dark slate background */
color: #f0e7db; /* Light text */
}
+
+html {
+ transition: background-color var(--transition-duration) ease-in-out,
+ color var(--transition-duration) ease-in-out;
+}Committable suggestion was skipped due to low confidence.
| :root { | ||
| --transition-duration: 500ms; | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Remove unused CSS variable --transition-duration
The --transition-duration CSS variable declared in frontend/src/index.css is not being used anywhere in the codebase. While there are many transition-related properties throughout the application, they all use hardcoded duration values (e.g., 300ms, 500ms) and none reference this variable using var(--transition-duration). Consider either:
- Removing this unused variable, or
- Refactoring existing transitions to use this variable for consistent animation timing
🔗 Analysis chain
Verify the usage of --transition-duration variable.
The new CSS variable appears to be intended for theme transition animations, but its usage is not visible in the provided changes.
Let's check if the variable is used in any way, even without the var() syntax, and also look for transition properties in general to understand the context better.
The previous search showed the variable declaration but we need to verify its usage across both CSS and JavaScript files, as it might be used in component styles or dynamic CSS.
Let's do a broader search to ensure we don't miss any usage of this variable, including in JavaScript/TypeScript files where it might be used in template literals or style objects.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the transition-duration variable
rg "var\(--transition-duration\)"
Length of output: 126
Script:
#!/bin/bash
# Search for any transition-related properties in CSS files
rg "transition" --type css
# Also search for the exact variable name without var() wrapper
rg "--transition-duration" --type css
Length of output: 627
Script:
#!/bin/bash
# Search for the variable with proper escaping
rg "\-\-transition\-duration" --type css
# Let's also check JavaScript/JSX files for potential usage
rg "transition" --type js --type jsx -A 2
Length of output: 167
Script:
#!/bin/bash
# Search in all files (not just css) to catch any potential usage
rg "\-\-transition\-duration"
# Search for transition-related properties in all frontend files
fd . frontend -t f -e js -e jsx -e tsx -e css | xargs rg "transition"
Length of output: 9617
| {/* Moon Icon for Light Mode */} | ||
| <FaMoon | ||
| className={`absolute left-1 w-5 h-5 text-gray-700 transform transition-all duration-300 ${ | ||
| isDarkMode ? 'opacity-0 scale-0' : 'opacity-100 scale-100' | ||
| }`} | ||
| /> | ||
|
|
||
| {/* Sun Icon for Dark Mode */} | ||
| <FaSun | ||
| className={`absolute right-1 w-5 h-5 text-gray-100 transform transition-all duration-300 ${ | ||
| isDarkMode ? 'opacity-100 scale-100' : 'opacity-0 scale-0' | ||
| }`} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor duplicate icon animation styles.
The sun and moon icons share similar animation logic with hardcoded transition durations. Consider extracting common styles and using the CSS variable.
+const iconBaseStyles = "absolute w-5 h-5 transform transition-all duration-[var(--transition-duration)]";
+const moonIconStyles = `${iconBaseStyles} left-1 text-gray-700`;
+const sunIconStyles = `${iconBaseStyles} right-1 text-gray-100`;
-<FaMoon
- className={`absolute left-1 w-5 h-5 text-gray-700 transform transition-all duration-300 ${
- isDarkMode ? 'opacity-0 scale-0' : 'opacity-100 scale-100'
- }`}
-/>
+<FaMoon
+ className={`${moonIconStyles} ${isDarkMode ? 'opacity-0 scale-0' : 'opacity-100 scale-100'}`}
+/>
-<FaSun
- className={`absolute right-1 w-5 h-5 text-gray-100 transform transition-all duration-300 ${
- isDarkMode ? 'opacity-100 scale-100' : 'opacity-0 scale-0'
- }`}
-/>
+<FaSun
+ className={`${sunIconStyles} ${isDarkMode ? 'opacity-100 scale-100' : 'opacity-0 scale-0'}`}
+/>📝 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.
| {/* Moon Icon for Light Mode */} | |
| <FaMoon | |
| className={`absolute left-1 w-5 h-5 text-gray-700 transform transition-all duration-300 ${ | |
| isDarkMode ? 'opacity-0 scale-0' : 'opacity-100 scale-100' | |
| }`} | |
| /> | |
| {/* Sun Icon for Dark Mode */} | |
| <FaSun | |
| className={`absolute right-1 w-5 h-5 text-gray-100 transform transition-all duration-300 ${ | |
| isDarkMode ? 'opacity-100 scale-100' : 'opacity-0 scale-0' | |
| }`} | |
| /> | |
| const iconBaseStyles = "absolute w-5 h-5 transform transition-all duration-[var(--transition-duration)]"; | |
| const moonIconStyles = `${iconBaseStyles} left-1 text-gray-700`; | |
| const sunIconStyles = `${iconBaseStyles} right-1 text-gray-100`; | |
| {/* Moon Icon for Light Mode */} | |
| <FaMoon | |
| className={`${moonIconStyles} ${isDarkMode ? 'opacity-0 scale-0' : 'opacity-100 scale-100'}`} | |
| /> | |
| {/* Sun Icon for Dark Mode */} | |
| <FaSun | |
| className={`${sunIconStyles} ${isDarkMode ? 'opacity-100 scale-100' : 'opacity-0 scale-0'}`} | |
| /> |
|
@AnushkaChouhan25 resolve the conflicts |
|
@RamakrushnaBiswal Done |
RamakrushnaBiswal
merged commit 7aa1cc1
into
RamakrushnaBiswal:main
RamakrushnaBiswal
added
level2
Fix issue- #411 and #412
Improve Footer Text Visibility in Light Theme and Change the theme toggle button design.
Summary by CodeRabbit
New Features
Bug Fixes
Style