Enhancement: Added Sliding Toggle Animation to Theme Switcher - #170
Conversation
📝 WalkthroughWalkthroughReplaces ThemeSwitch's button-based UI with a new reusable Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Consumer (Nav / Settings)
participant ThemeSwitch as ThemeSwitch
participant GlassSwitch as GlassSwitch
participant Tooltip as Tooltip
participant ThemeHook as useTheme
rect rgba(220,235,255,0.9)
UI->>ThemeSwitch: render
ThemeSwitch->>ThemeHook: read current theme (isDark)
ThemeSwitch->>GlassSwitch: render props (isOn, ariaLabel, thumbContent, onChange)
GlassSwitch->>Tooltip: wrap with ariaLabel
end
rect rgba(235,255,230,0.9)
GlassSwitch->>GlassSwitch: user click / Enter/Space
GlassSwitch->>ThemeSwitch: onChange()
ThemeSwitch->>ThemeHook: toggleTheme()
ThemeHook-->>ThemeSwitch: new theme state
ThemeSwitch->>GlassSwitch: update isOn / ariaLabel
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/components/ThemeSwitch.jsx (1)
24-24: Icon logic is correct; clarify animation expectations.The conditional rendering correctly shows the Sun icon in dark mode and Moon icon in light mode, with appropriate CSS classes for positioning. However, note that this creates a toggle-like visual by positioning icons differently rather than animating a single element sliding between positions. When themes switch, the icons are replaced, not smoothly transitioned.
If a true sliding animation is desired (one element moving from left to right), consider using a single wrapper element with a transform transition instead of conditionally rendering different icons.
💡 Alternative approach for true sliding animation
<button aria-label={`Switch to ${theme === 'dark' ? "Light" : "Dark"} Mode`} type="button" className={`glass ${styles.themeButton} ${theme === 'light' ? styles.themeButtonLight : styles.themeButtonDark}`} onClick={toggleTheme} > <div className={`${styles.iconWrapper} ${theme === 'dark' ? styles.iconLeft : styles.iconRight}`}> {theme === 'dark' ? <Sun className={styles.icon} /> : <Moon className={styles.icon} />} </div> </button>Then in CSS:
.iconWrapper { position: absolute; top: 50%; transform: translateY(-50%); transition: all 0.3s ease-in-out; } .iconLeft { left: 6px; } .iconRight { right: 6px; }This approach animates the position change when switching themes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
src/components/ThemeSwitch.jsxsrc/components/ThemeSwitch.module.css
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ThemeSwitch.jsx (1)
src/hooks/useTheme.js (2)
theme(4-17)toggleTheme(37-39)
🔇 Additional comments (1)
src/components/ThemeSwitch.jsx (1)
21-22: Theme-dependent button styling looks correct.The conditional className logic properly applies
themeButtonLightin light mode andthemeButtonDarkin dark mode. Theglassclass is appropriately combined, and thearia-labelenhances accessibility.Note: This implementation depends on the CSS fixes suggested in
ThemeSwitch.module.css. If you adopt the refactoring to extract common button styles, update the className accordingly.
| .themeButton:hover { | ||
| .icon { | ||
| color: var(--color-secondary); | ||
| } | ||
| transform: scale(1.05); | ||
| transform: scale(1.02); | ||
| } |
There was a problem hiding this comment.
Critical: Hover selector references non-existent class.
The .themeButton:hover selector won't work because .themeButton was renamed to .themeButtonLight and .themeButtonDark. The hover effect is currently non-functional.
🔎 Proposed fix for hover selector
-.themeButton:hover {
+.themeButtonLight:hover,
+.themeButtonDark:hover {
.icon {
color: var(--color-secondary);
}
transform: scale(1.02);
}Alternatively, if you adopt the refactoring suggested in the previous comment, you can use:
.themeButton:hover {
.icon,
.icondark {
color: var(--color-secondary);
}
transform: scale(1.02);
}Note: I've also added .icondark to the nested selector so the moon icon also changes color on hover.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/components/ThemeSwitch.module.css around lines 27 to 32, the hover rule
targets .themeButton which no longer exists (renamed to .themeButtonLight and
.themeButtonDark), so the hover effects don't apply; update the selector to
target the actual button classes (e.g., .themeButtonLight:hover,
.themeButtonDark:hover) and include both icon class variants inside the hover
(e.g., .icon and .icondark) so the icon color and transform are applied on hover
for both light and dark buttons.
|
I’ve pushed updates addressing the review feedback , refactored duplicated styles, fixed hover selectors, improved icon sizing/positioning, and removed hard-coded values. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/components/ThemeSwitch.jsx (1)
21-21: Minor: Capitalize "UI" in comment.The comment uses lowercase "ui" which should be "UI" for proper capitalization of the acronym.
🔎 Suggested fix
- {/* Added sliding toggle effect to change theme mode which enhances ui */} + {/* Added sliding toggle effect to change theme mode which enhances the UI */}src/components/ThemeSwitch.module.css (3)
1-13: Good: DRY refactoring addresses past feedback.The common button properties have been properly extracted into the base
.themeButtonclass, resolving the duplication flagged in previous reviews. The flexbox centering approach is appropriate for icon positioning.Minor formatting note: Line 9 is missing a space after the colon (
position:relativeshould beposition: relative).Based on learnings from past reviews about DRY violations.
23-29: Consider using CSS variables for hover colors.The hover color changes use hard-coded values (
rgb(220, 220, 220)and#1a001f) instead of leveraging existing CSS variables likevar(--color-primary)orvar(--color-secondary). This reduces maintainability and may cause contrast issues if the theme color scheme changes.🔎 Suggested refactor
.themeButton:hover .icondark { - color: rgb(220, 220, 220); + color: var(--color-secondary); } .themeButton:hover .icon { - color: #1a001f; + color: var(--color-secondary); }
50-62: Transform logic is correct for sliding animation.The percentage-based
translateXtransforms are well-designed and adaptive to icon size changes. Once both icons are rendered simultaneously (see critical issue in ThemeSwitch.jsx), these transforms will properly create the sliding toggle effect.Minor formatting: Line 54 has inconsistent indentation—it should have leading spaces to match the other transform rules.
🔎 Formatting fix
.themeButtonLight .icondark{ -transform: translateX(100%); + transform: translateX(100%); }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ThemeSwitch.jsxsrc/components/ThemeSwitch.module.css
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ThemeSwitch.jsx (1)
src/hooks/useTheme.js (2)
theme(4-17)toggleTheme(37-39)
🔇 Additional comments (5)
src/components/ThemeSwitch.jsx (1)
22-22: LGTM: Button className logic is correct.The conditional application of theme-specific classes is implemented correctly, and the base
styles.themeButtonclass is properly included.src/components/ThemeSwitch.module.css (4)
15-21: LGTM: Clean variant separation.The theme variants now only specify their respective background colors, completing the DRY refactoring. The approach is maintainable and follows the single responsibility principle.
31-34: LGTM: Appropriate hover feedback.The cursor change and subtle scale transform provide good visual feedback for user interaction.
37-48: Icon sizing and transitions are appropriate.The 1.3rem icon dimensions fit well within the 30px button height when centered via flexbox. While
.iconand.icondarkshare identical properties (which could suggest a DRY opportunity), maintaining separate classes is necessary for the distinct transform rules applied in theme-specific selectors below.
64-70: Excellent: Accessibility support for reduced motion.The
prefers-reduced-motionmedia query properly respects user preferences for reduced animations, which is an important accessibility consideration. This is a best practice implementation.
| <button aria-label={`Switch to ${ theme === 'dark' ? "Light" : "Dark" } Mode`} type="button" className={`glass ${styles.themeButton} ${theme === 'light' ? styles.themeButtonLight : styles.themeButtonDark}`} onClick={toggleTheme}> | ||
| {/* Show Sun icon in dark mode, Moon icon in light mode */} | ||
| {theme === 'dark' ? <Sun className={styles.icon} /> : <Moon className={styles.icon} />} | ||
| {theme === 'dark' ? <Sun className={styles.icon} /> : <Moon className={styles.icondark} />} |
There was a problem hiding this comment.
Critical: Sliding animation cannot work with conditional rendering.
The JSX only renders one icon at a time using a ternary operator, but the CSS animation (lines 50-62 in ThemeSwitch.module.css) expects both icons to be present simultaneously in the DOM so they can slide via translateX transforms. Currently, icons will just appear/disappear instantly instead of sliding.
To achieve the sliding toggle effect, both icons must be rendered together, and the CSS will control their positions:
🔎 Proposed fix to render both icons
- {theme === 'dark' ? <Sun className={styles.icon} /> : <Moon className={styles.icondark} />}
+ <Sun className={styles.icon} />
+ <Moon className={styles.icondark} />This allows the CSS transforms to slide both icons in and out of view smoothly. The translateX rules in your CSS will handle showing/hiding the appropriate icon based on the theme.
📝 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.
| {theme === 'dark' ? <Sun className={styles.icon} /> : <Moon className={styles.icondark} />} | |
| <Sun className={styles.icon} /> | |
| <Moon className={styles.icondark} /> |
🤖 Prompt for AI Agents
In src/components/ThemeSwitch.jsx around line 24, the current ternary renders
only one icon at a time which prevents the CSS sliding animation from working;
change the JSX to render both <Sun> and <Moon> simultaneously and assign
classNames that toggle an "active" or "hidden" modifier based on the theme
(e.g., className={`${styles.icon} ${theme === 'dark' ? styles.active :
styles.inactive}`}) so the CSS can apply translateX transforms to slide icons
in/out; keep the existing click/role handlers and aria attributes intact and
ensure the container preserves both icons in the DOM for the animation to work.
|
Thanks for pointing this out , good catch. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/ThemeSwitch.jsx (2)
21-21: Optional: Capitalize "UI" in the comment.Minor grammar suggestion: "ui" should be "UI" (abbreviation for User Interface).
🔎 Suggested change
- {/* Added sliding toggle effect to change theme mode which enhances ui */} + {/* Added sliding toggle effect to change theme mode which enhances the UI */}
22-28: Excellent fix! Both icons now render, enabling the sliding animation.The change to render both
<Sun>and<Moon>icons unconditionally (lines 26-27) correctly addresses the critical issue from the previous review. The CSS can now applytranslateXtransforms to create the smooth sliding toggle effect. The dynamic className toggling on line 25 properly controls the visual state.Optional: Consider improving formatting and consistency.
A few minor polish suggestions for readability:
- Line 25: The className expression is quite long; consider breaking it across lines or extracting the ternary to a variable for clarity.
- Spacing: Inconsistent spacing in template literal—
${ themehas a space after${, but the outer${styles.themeButton}does not.- Quotes: Mixed quote styles—line 20 uses
'dark', while lines 23 and 25 use"dark". Standardizing on single or double quotes improves consistency.🔎 Example refactor for consistency
<button - aria-label={`Switch to ${theme === "dark" ? "Light" : "Dark"} Mode`} + aria-label={`Switch to ${theme === 'dark' ? 'Light' : 'Dark'} Mode`} type="button" - className={`${styles.themeButton} ${ theme === "dark" ? styles.themeButtonDark : styles.themeButtonLight }`} onClick={toggleTheme}> + className={`${styles.themeButton} ${theme === 'dark' ? styles.themeButtonDark : styles.themeButtonLight}`} + onClick={toggleTheme} +> <Sun className={styles.icon} /> <Moon className={styles.icondark} /> </button>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/ThemeSwitch.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ThemeSwitch.jsx (1)
src/hooks/useTheme.js (2)
theme(4-17)toggleTheme(37-39)
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
Hi @codevory. Thank you for your contribution - the community appreciates it!🦔
This is very nice, but it appears to have a few bugs and doesn't fully align with #123. I'll explain why below:
Bugs & Inconsistencies
The ThemeSwitch component appears to display both lucide-react icons regardless of the current theme and uses the incorrect backgrounds between the two themes:
Additionally (as you can see), the icons don't fit in the switch properly. The fix for that will be discussed in the next section.👇
Alignment with #123
#123 specifically asks for a reusable GlassSwitch - this is because it will allow the switch to be used anywhere inside the application, thereby decoupling it from specific implementations (such as with the ThemeSwitch) and reducing code repetition.
The bugs the current implementation currently faces could be fixed more easily through a modular GlassSwitch component since individual tests can be written for it and bugs can be addressed more easily since it will be a simpler component.
Summary of Requested Changes
- Please create a new file,
GlassCard.jsx, insidesrc/componentsand use the globalglassclass, which can be found insidesrc/global-styles/components.css.- It would be a good idea to have a look at the existing glass components (such as
GlassCard) to understand how to implement thisGlassSwitch. - Using the global styles will prevent CSS conflicts and ensure the component fits in well with the rest of the UI. This is the fix for the background problem.
- It would be a good idea to have a look at the existing glass components (such as
- If you are comfortable with it and have created the
GlassSwitch, please write some:- Tests for the new
GlassSwitchcomponent (in a file next toGlassSwitch.jsx, calledGlassSwitch.test.jsx) to ensure it functions properly (we use Vitest). - Documentation in the
docs/docsfolder (it uses Docusaurus's MDX, which is a blend between Markdown and React).
- Tests for the new
Ask Questions
I understand that you're new to open-source, so it would make sense if you got stuck or were confused by something. I'm here to help if you need me and you can also ask @CodeRabbit for help (it's an LLM that is familiar with this repository) - you've probably already seen the reviews it leaves on pull requests.
Be careful with @CodeRabbit, though - it is AI, meaning that it can be, and is often, wrong, so use your own judgement to make sure it isn't recommending something over-the-top or completely wrong.
There is also a Q&A discussion if you'd like help from the community.
Additional Resources
General
I'm sure you've already been through the introduction page that helps with setting up your local environment. After that, you should make sure to read through Img2Num's guidelines .
Documentation
We have documentation on how to write documentation in Docusaurus, but that could be a bit too minimal depending on your circumstances. In that case, the official Docusaurus documentation is the best place to go if you need help with writing docs.
Tests
Vitest's documentation is the best place to go for this since Img2Num does not have a guide on how to write tests yet. @CodeRabbit can also help you with writing tests if you need.
I'm sorry for writing so much - I hope it wasn't a problem. Thanks again for the contribution and please feel free to ask for help when you need it. Have a good day!🦔🦔
|
Hi @Ryan-Millard, thank you so much for the detailed explanation and for taking the time to point these things out , I really appreciate it. That makes sense now. I see that my current implementation improves the ThemeSwitch, but doesn’t fully align with #123, which is about creating a reusable GlassSwitch component. The expectation is to create a reusable GlassSwitch using the global glass styles I’m happy to continue working on this and would appreciate any guidance on whether it’s okay to focus on the component first, and then look into tests and documentation once the core behavior is correct. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/components/NavBar.jsx (2)
8-9: Use consistent import aliases.Line 8 uses a relative import
'./GlassSwitch'while other component imports use the@components/alias. For consistency with the existing codebase pattern (lines 5-7), use'@components/GlassSwitch'.🔎 Proposed fix
-import GlassSwitch from './GlassSwitch'; +import GlassSwitch from '@components/GlassSwitch'; import { useTheme } from '@hooks/useTheme';
15-16: Fix spacing in object destructuring.Line 15 has inconsistent spacing:
{theme , toggleTheme}has a space before the comma but standard formatting requires no space before and a space after.🔎 Proposed fix
- const {theme , toggleTheme} = useTheme() + const { theme, toggleTheme } = useTheme(); const isDark = theme === 'dark';src/components/GlassSwitch.module.css (2)
11-17: Use more compatible flexbox values.Lines 12 and 15 use
justify-content: startandjustify-content: end. While valid,flex-startandflex-endhave broader browser support and are more explicit for flexbox contexts.🔎 Proposed fix
.unChecked{ - justify-content: start; + justify-content: flex-start; } .checked{ - justify-content: end; + justify-content: flex-end; background-color: rgb(26, 194, 26); }
1-25: Add accessibility features and improve maintainability.The CSS is missing important accessibility and maintainability features:
No focus styles: Users navigating with keyboards need visible focus indicators. Add focus/focus-visible styles to
.switch.No reduced-motion support: Users with vestibular disorders who have
prefers-reduced-motion: reduceenabled should see instant state changes, not animations.Hard-coded values: Magic numbers like
64px,32px,30px,20pxmake the component harder to maintain. Consider using CSS custom properties.🔎 Proposed improvements
.switch{ display: flex; align-items: center; width:64px; height:32px; border-radius: 30px; padding: 0; margin-right:20px; cursor: pointer; } + +.switch:focus-visible { + outline: 2px solid currentColor; + outline-offset: 2px; +} .thumb{ width:30px; height:30px; border-radius: 100%; transition:color 0.3s ease; background-color: rgba(255, 255, 255, 0.956); } + +@media (prefers-reduced-motion: reduce) { + .thumb { + transition: none; + } +}src/components/GlassSwitch.jsx (2)
3-3: Fix spacing in parameter destructuring.The parameters have inconsistent spacing:
{onChange ,checked,ariaLabel}has a space before the first comma but no spaces after any commas. Standard JavaScript formatting requires no space before commas and a space after each comma.🔎 Proposed fix
-const GlassSwitch = ({onChange ,checked,ariaLabel}) => { +const GlassSwitch = ({ onChange, checked, ariaLabel }) => {
3-11: Consider adding prop validation.The
GlassSwitchcomponent lacks prop validation. Adding PropTypes (or TypeScript types if migrating) would improve type safety and developer experience by catching incorrect prop usage.🔎 Proposed addition using PropTypes
import styles from './GlassSwitch.module.css' +import PropTypes from 'prop-types'; -const GlassSwitch = ({onChange ,checked,ariaLabel}) => { +const GlassSwitch = ({ onChange, checked, ariaLabel }) => { return ( <button type='button' role='switch' onClick={onChange} aria-checked={checked} className={`glass ${styles.switch} ${checked ? styles.checked : styles.unChecked}`} aria-label={ariaLabel}> <span type='span' className={`${styles.thumb}`}></span> </button> ) } +GlassSwitch.propTypes = { + onChange: PropTypes.func.isRequired, + checked: PropTypes.bool.isRequired, + ariaLabel: PropTypes.string.isRequired, +}; + export default GlassSwitchNote: You'll need to install
prop-typesif not already present:npm install prop-types
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
package.jsonsrc/components/GlassSwitch.jsxsrc/components/GlassSwitch.module.csssrc/components/GlassSwitch.test.jsxsrc/components/NavBar.jsxsrc/components/ThemeSwitch.jsxsrc/components/ThemeSwitch.module.csssrc/components/ThemeSwitch.test.jsx
✅ Files skipped from review due to trivial changes (2)
- src/components/GlassSwitch.test.jsx
- src/components/ThemeSwitch.test.jsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/ThemeSwitch.jsx
- src/components/ThemeSwitch.module.css
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/NavBar.jsx (3)
src/components/ThemeSwitch.jsx (2)
useTheme(17-17)ThemeSwitch(15-27)src/hooks/useTheme.js (3)
useTheme(3-42)theme(4-17)toggleTheme(37-39)src/components/GlassSwitch.jsx (1)
GlassSwitch(3-11)
🔇 Additional comments (1)
package.json (1)
162-162: Both dependency versions are verified to exist on the npm registry and have no known security vulnerabilities:
- @tanstack/react-query@5.90.14 ✓
- jsdom@27.4.0 ✓
The version updates are safe to proceed with.
|
Hi @Ryan-Millard 👋 I’ve made the requested changes and would appreciate a review when you have time.
What I did not change
The GlassSwitch is now fully reusable and can be used for other toggles (e.g. mobile menu, feature flags) without modification. |
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
This is exactly what we needed. Thank you!
I left individual comments on some of the files in this PR, so please will you address them as well as the below general ones:
- Please add some documentation in the
docs/folder - Fix the formatting inconsistencies in your code (like the
GlassSwitchprops and the CSS file).
The merge-base changed after approval.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/components/NavBar.jsx (2)
15-15: Fix spacing in object destructuring.Inconsistent spacing: space before comma but not after in
{theme , toggleTheme}. JavaScript style guides typically prefer no space before comma and space after.🔎 Proposed fix
- const {theme , toggleTheme} = useTheme() + const { theme, toggleTheme } = useTheme();Note: Also added the missing semicolon for consistency.
63-63: Fix double spacing between props.There are two spaces between
isOn={isDark}andonChange={toggleTheme}.🔎 Proposed fix
- <GlassSwitch isOn={isDark} onChange={toggleTheme} thumbContent={isDark ? <Moon /> : <Sun />} ariaLabel={`switch to ${isDark ? 'light' : 'dark'} mode`} /> + <GlassSwitch isOn={isDark} onChange={toggleTheme} thumbContent={isDark ? <Moon /> : <Sun />} ariaLabel={`switch to ${isDark ? 'light' : 'dark'} mode`} />src/components/GlassSwitch.jsx (1)
23-28: Consider makingisOnprop required.The
isOnprop is currently optional in PropTypes (line 24), but the component's behavior and ARIA state depend on it. For a controlled component where the parent must manage state, making this prop required would catch integration errors earlier and align with the controlled-component pattern described in the documentation.🔎 Proposed fix
GlassSwitch.propTypes = { - isOn: PropTypes.bool, + isOn: PropTypes.bool.isRequired, onChange: PropTypes.func.isRequired, ariaLabel: PropTypes.string.isRequired, thumbContent: PropTypes.node, };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
docs/docs/components/glass-switch.mdxpackage.jsonsrc/components/GlassSwitch.jsxsrc/components/GlassSwitch.module.csssrc/components/GlassSwitch.test.jsxsrc/components/NavBar.jsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/GlassSwitch.test.jsx
- src/components/GlassSwitch.module.css
🧰 Additional context used
🧬 Code graph analysis (2)
src/components/NavBar.jsx (2)
src/hooks/useTheme.js (3)
useTheme(3-42)theme(4-17)toggleTheme(37-39)src/components/GlassSwitch.jsx (1)
GlassSwitch(5-21)
src/components/GlassSwitch.jsx (1)
src/components/Tooltip.jsx (1)
Tooltip(5-44)
🔇 Additional comments (1)
src/components/GlassSwitch.jsx (1)
5-21: Well-implemented accessible switch component.The implementation correctly uses
role="switch"andaria-checkedwith string values ('true'/'false') per ARIA specifications. The button element provides keyboard accessibility by default (Space and Enter keys), and the Tooltip wrapper enhances discoverability. The controlled component pattern is clean and the separation of presentation from business logic aligns with the stated objectives.Minor note: Both the Tooltip content (line 8) and button's
aria-label(line 15) use the sameariaLabelvalue. This redundancy is harmless but thearia-labelalone would typically suffice for screen readers. The Tooltip provides visual feedback for sighted users on hover, so keeping both is acceptable for enhanced UX.
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
This is really pretty!
Your documentation is excellent - I love that you added extra resources. That's a sign of good documentation and I appreciate it.
There was a problem hiding this comment.
Is this file still needed? I can't access it.😅
There was a problem hiding this comment.
i have deleted that file ( docs/docs/components/glass-switch.mdx )
Although i have improved that documentation file & moved to docs/docs/reference/react/components/GlassSwitch/index.md
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/GlassSwitch.jsx (2)
6-6: Remove extra space before semicolon.There's an unnecessary space before the semicolon at the end of line 6.
🔎 Proposed fix
- const fallbackContent = isOn ? styles.fallbackThumbContentOn : styles.fallbackThumbContentOff ; + const fallbackContent = isOn ? styles.fallbackThumbContentOn : styles.fallbackThumbContentOff;
31-31: Add space after colon for consistency.Line 31 is missing a space after the colon, which is inconsistent with the formatting on other lines.
🔎 Proposed fix
- disabled:PropTypes.bool, + disabled: PropTypes.bool,
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/docs/reference/react/components/GlassSwitch/index.mdsrc/components/GlassSwitch.jsxsrc/components/ThemeSwitch.module.css
💤 Files with no reviewable changes (1)
- src/components/ThemeSwitch.module.css
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/GlassSwitch.jsx (1)
src/components/Tooltip.jsx (1)
Tooltip(5-44)
🪛 markdownlint-cli2 (0.18.1)
docs/docs/reference/react/components/GlassSwitch/index.md
99-99: Table pipe style
Expected: leading_and_trailing; Actual: leading_only; Missing trailing pipe
(MD055, table-pipe-style)
|
@Ryan-Millard Thank you so much for the review and kind words , I really appreciate it! This was a great learning experience for me. The feedback around single responsibility, component architecture, and documentation quality helped me understand how real-world open-source projects are structured and maintained. I’m especially glad the documentation met expectations . I tried to make it clear and useful for future contributors as well. |
Hey @codevory, I’m really glad to hear that! In a nutshell, good code aims to build loosely-coupled, reusable components that are simple enough to explain in a single sentence. When I was learning to code, I struggled with this too—until I read Programming: Principles and Practice Using C++ by Bjarne Stroustrup (the creator of C++). While it isn’t directly about modern React, it covers core concepts like algorithms & data structures, design thinking, and decomposition & abstraction—all of which are crucial for building maintainable projects. Regarding the documentation, you absolutely nailed it! It was clear and logically structured, which will make life easier for future contributors. Thanks again for your contribution—I’m looking forward to seeing more from you soon! 🦔 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/ThemeSwitch.test.jsx (1)
85-93: Incomplete keyboard accessibility test—missing Space/Enter interaction.The PR objectives explicitly require "keyboard focusable and toggleable via Space/Enter." This test only verifies focusability but doesn't test that pressing Space or Enter actually toggles the switch.
🔎 Proposed fix: Add keyboard interaction tests
Add new test cases for Space and Enter keys:
it('is keyboard accessible (focusable)', () => { vi.spyOn(useThemeModule, 'useTheme').mockReturnValue({ theme: 'light', toggleTheme: mockToggleTheme }); render(<ThemeSwitch />); const button = screen.getByRole('switch', { name: 'switch to dark mode' }); button.focus(); expect(button).toHaveFocus(); }); + it('toggles theme when Space key is pressed', () => { + vi.spyOn(useThemeModule, 'useTheme').mockReturnValue({ theme: 'light', toggleTheme: mockToggleTheme }); + + render(<ThemeSwitch />); + + const button = screen.getByRole('switch', { name: 'switch to dark mode' }); + fireEvent.keyDown(button, { key: ' ', code: 'Space' }); + + expect(mockToggleTheme).toHaveBeenCalledTimes(1); + }); + + it('toggles theme when Enter key is pressed', () => { + vi.spyOn(useThemeModule, 'useTheme').mockReturnValue({ theme: 'light', toggleTheme: mockToggleTheme }); + + render(<ThemeSwitch />); + + const button = screen.getByRole('switch', { name: 'switch to dark mode' }); + fireEvent.keyDown(button, { key: 'Enter', code: 'Enter' }); + + expect(mockToggleTheme).toHaveBeenCalledTimes(1); + });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/docs/reference/react/components/ThemeSwitch/tests.mdpackage.jsonsrc/components/ThemeSwitch.test.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/docs/reference/react/components/ThemeSwitch/tests.md
🧰 Additional context used
🧬 Code graph analysis (1)
src/components/ThemeSwitch.test.jsx (1)
src/components/ThemeSwitch.jsx (1)
ThemeSwitch(14-27)
🔇 Additional comments (2)
package.json (2)
166-166: LGTM! The prop-types addition is correctly versioned and necessary.The addition of
prop-types@15.8.1is required for the new GlassSwitch component's PropTypes validation. The version is the latest stable release with no known security vulnerabilities, as confirmed by prior verification.
175-175: LGTM! The version correction was properly applied.The update to
@tanstack/react-query@5.90.12correctly addresses the previous review feedback regarding the non-existent 5.90.14 version. This is the latest stable release with no known security vulnerabilities.
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
Excellent work! Thank you so much. I hope we see more contributions from you in the future!🦔🦔
…witch components, tests, and docs
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
docs/docs/reference/react/components/GlassSwitch/tests.md (1)
9-11: Add language identifier to fenced code block.The fenced code block should specify
bashor an appropriate language identifier for better syntax highlighting and tooling support.🔎 Proposed fix
-``` +```bash src/components/GlassSwitch.test.jsx</details> </blockquote></details> <details> <summary>src/components/GlassSwitch.test.jsx (1)</summary><blockquote> `49-62`: **Consider adding explicit test coverage for Space key activation.** The keyboard accessibility test verifies Enter key functionality, but according to issue #123 requirements, the switch should be "toggleable via Space/Enter." While the native `<button>` element inherently supports both keys, explicitly testing Space key activation would provide complete coverage of the stated accessibility requirements. <details> <summary>🔎 Proposed addition</summary> Add a test case for Space key activation: ```javascript it('responds to Space key', async () => { const user = userEvent.setup(); const onChange = vi.fn(); render(<GlassSwitch isOn={false} onChange={onChange} ariaLabel="Toggle" />); const button = screen.getByRole('switch'); button.focus(); await user.keyboard(' '); expect(onChange).toHaveBeenCalled(); });
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
docs/docs/reference/react/components/GlassSwitch/index.mddocs/docs/reference/react/components/GlassSwitch/tests.mddocs/docs/reference/react/components/ThemeSwitch/index.mddocs/docs/reference/react/components/ThemeSwitch/tests.mdsrc/components/GlassSwitch.jsxsrc/components/GlassSwitch.module.csssrc/components/GlassSwitch.test.jsxsrc/components/ThemeSwitch.jsxsrc/components/ThemeSwitch.test.jsx
✅ Files skipped from review due to trivial changes (1)
- docs/docs/reference/react/components/GlassSwitch/index.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/ThemeSwitch.jsx
- src/components/GlassSwitch.module.css
- src/components/ThemeSwitch.test.jsx
- src/components/GlassSwitch.jsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-04T15:34:04.654Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 146
File: docs/docs/reference/react/components/ThemeSwitch/tests.md:123-124
Timestamp: 2026-01-04T15:34:04.654Z
Learning: In Docusaurus documentation, relative directory links (e.g., ../ or ../../../hooks/useTheme) resolve to index.md within the target directory. Do not require or force linking to index.md explicitly; such relative links remain valid and should not be changed when reviewing docs under the docs directory.
Applied to files:
docs/docs/reference/react/components/ThemeSwitch/tests.mddocs/docs/reference/react/components/ThemeSwitch/index.mddocs/docs/reference/react/components/GlassSwitch/tests.md
🧬 Code graph analysis (1)
src/components/GlassSwitch.test.jsx (1)
src/components/GlassSwitch.jsx (1)
GlassSwitch(5-21)
🪛 LanguageTool
docs/docs/reference/react/components/GlassSwitch/tests.md
[style] ~54-~54: To form a complete sentence, be sure to include a subject.
Context: ...ed ### 6. Component props (2 tests) - Can be disabled via disabled prop - Sets ...
(MISSING_IT_THERE)
🪛 markdownlint-cli2 (0.18.1)
docs/docs/reference/react/components/GlassSwitch/tests.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (2)
docs/docs/reference/react/components/ThemeSwitch/tests.md (1)
5-115: LGTM! Documentation accurately reflects the GlassSwitch refactor.The test documentation has been properly updated to reflect the migration from button-based ThemeSwitch to GlassSwitch-based implementation, including:
- Updated switch semantics (role="switch", aria-checked)
- Correct aria-label patterns reflecting the next theme state
- Test snippets consistent with the refactored implementation
- Appropriate mocking strategy for the new dependencies
docs/docs/reference/react/components/ThemeSwitch/index.md (1)
1-176: Excellent documentation update for the GlassSwitch refactor!The documentation thoroughly and accurately reflects the migration to GlassSwitch-based implementation, including:
- Clear explanation of the architectural change (delegation to GlassSwitch)
- Updated accessibility table with specific focus styling details meeting WCAG 2.4.7
- Comprehensive code examples showing the new GlassSwitch integration
- Consistent aria-label patterns throughout all examples
- Well-documented styling approach and customization options
The documentation provides excellent guidance for developers using ThemeSwitch and clearly communicates the component's behavior and accessibility features.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/docs/reference/react/components/GlassSwitch/tests.md
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-04T15:34:04.654Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 146
File: docs/docs/reference/react/components/ThemeSwitch/tests.md:123-124
Timestamp: 2026-01-04T15:34:04.654Z
Learning: In Docusaurus documentation, relative directory links (e.g., ../ or ../../../hooks/useTheme) resolve to index.md within the target directory. Do not require or force linking to index.md explicitly; such relative links remain valid and should not be changed when reviewing docs under the docs directory.
Applied to files:
docs/docs/reference/react/components/GlassSwitch/tests.md
🪛 LanguageTool
docs/docs/reference/react/components/GlassSwitch/tests.md
[style] ~56-~56: To form a complete sentence, be sure to include a subject.
Context: ...ded and isOn ### 6. Component props - Can be disabled via disabled prop - Sets ...
(MISSING_IT_THERE)
🪛 markdownlint-cli2 (0.18.1)
docs/docs/reference/react/components/GlassSwitch/tests.md
9-9: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (1)
docs/docs/reference/react/components/GlassSwitch/tests.md (1)
5-170: Documentation is clear and well-organized.The test documentation effectively covers test organization, mocking strategy, practical examples, and coverage areas. The content aligns with the GlassSwitch component's test suite and provides readers with actionable guidance for running tests and understanding coverage. The relative links to related documentation (e.g.,
./index.mdand../../../guidelines/testing.md) are properly formatted for Docusaurus. Based on learnings, relative directory links in the docs resolve correctly without requiring explicit index.md references.
|
Thank you again for the great work, @codevory. I hope you have a great day! |
* added a sliding ui effect for theme switcher button * refactor(theme-switch): address review feedback * fix(theme-switch): render both icons to enable sliding animation * refactor(glass-switch): apply DRY styles and finalize reusable switch * fix(glass-switch): restore icon animation after merge * fixed some issues & added sliding effect * feat(glass-switch): finalize reusable switch with tests and accessibility * docs: add GlassSwitch component documentation * fixed changes suggested by coderabbit * refactor(theme-switch): move theme logic back into ThemeSwitch to follow SRP * removed unused code & rendered themeswitch only * added isRequired to isOn * Update @tanstack/react-query to the latest stable version (5.90.12). * added size , disabled props that are optional * improved documentation for GlassSwitch resuable component * Added documentation for GlassSwitch component including it's tests * Improved documentation as per it's usage & also modified it's test documentation accordingly * Added a fallback Content when no thumbContent is given to component * Improved the tests for GlassSwitch component to address changes * Added useful comments back * Improved tests for ThemeSwitch component & Adresed the required changes as per it's new usage * modified documentation to ease the usage * Fixed missing focus indicator in GlassSwitch component. * Fixed extra space issue * feat(GlassSwitch): improve styles * removed size props as is not being used * fixed table formating issue * Fixed react node issue in classname * style(react: GlassSwitch, ThemeSwitch): format GlassSwitch and ThemeSwitch components, tests, and docs * docs(react: GlassSwitch): fix wording * fix(docs: broken links): fix links in GlassSwitch documentation --------- Co-authored-by: Ryan-Millard <millardryandevon@gmail.com> Co-authored-by: Ryan Millard <142347829+Ryan-Millard@users.noreply.github.com>
* added a sliding ui effect for theme switcher button * refactor(theme-switch): address review feedback * fix(theme-switch): render both icons to enable sliding animation * refactor(glass-switch): apply DRY styles and finalize reusable switch * fix(glass-switch): restore icon animation after merge * fixed some issues & added sliding effect * feat(glass-switch): finalize reusable switch with tests and accessibility * docs: add GlassSwitch component documentation * fixed changes suggested by coderabbit * refactor(theme-switch): move theme logic back into ThemeSwitch to follow SRP * removed unused code & rendered themeswitch only * added isRequired to isOn * Update @tanstack/react-query to the latest stable version (5.90.12). * added size , disabled props that are optional * improved documentation for GlassSwitch resuable component * Added documentation for GlassSwitch component including it's tests * Improved documentation as per it's usage & also modified it's test documentation accordingly * Added a fallback Content when no thumbContent is given to component * Improved the tests for GlassSwitch component to address changes * Added useful comments back * Improved tests for ThemeSwitch component & Adresed the required changes as per it's new usage * modified documentation to ease the usage * Fixed missing focus indicator in GlassSwitch component. * Fixed extra space issue * feat(GlassSwitch): improve styles * removed size props as is not being used * fixed table formating issue * Fixed react node issue in classname * style(react: GlassSwitch, ThemeSwitch): format GlassSwitch and ThemeSwitch components, tests, and docs * docs(react: GlassSwitch): fix wording * fix(docs: broken links): fix links in GlassSwitch documentation --------- Co-authored-by: Ryan-Millard <millardryandevon@gmail.com> Co-authored-by: Ryan Millard <142347829+Ryan-Millard@users.noreply.github.com>
2 participants
Enhancement: Added Sliding Toggle Animation to Theme Switcher
Fixes #123
This PR improves the existing theme switcher by adding a smooth sliding toggle animation, making the interaction more intuitive and visually engaging.
While working on this feature, I noticed that a basic theme switcher had already been implemented. Instead of duplicating functionality, I focused on enhancing the user experience by introducing a sliding effect that better communicates the theme change (light ↔ dark).
As this is my first open-source contribution, it took me around 5–6 hours to understand the codebase, align with the existing implementation, and integrate the animation cleanly.
I hope this improvement adds value to the UI/UX.
Feedback, suggestions, or improvements are very welcome , I’d love to learn and iterate further!
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.