Changed light mode icon because it was misleading #2742
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughThis PR introduces a new Sun icon component to replace a misleading sun icon in the theme toggle. The ModeToggle component is updated to conditionally render the new Sun component for dark mode and a FontAwesome moon icon for light mode, with updated styling. Test mocks and Jest coverage configuration are updated accordingly. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
|
@dhirukumar why did you close previous PR? 🤔 This makes it much harder to review since reviewer leaves comments to be addressed. I'll leave this one be for this first time, but next time I'll just close this duplicate. |
|
Hi @kasya , sorry for the inconvenience caused. I closed the previous PR because I was also working on the Skeleton feature at the same time, and switching branches with stashes caused some files to get mixed into the PR that weren’t related to the actual change. While running make check-test, some extra files were added for making the check test pass, and I got a bit stuck. Thank you for pointing it out. |
|
Hey! Not sure if it’s my place to say 😅, but a light-bulb icon can be a bit confusing here since in many modern UIs it usually represents tips/help/insights. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/icons/Sun.tsx (1)
1-25: Solid implementation; consider readonly props, hoistingpaths, and a11y tweaksThe component looks correct and fits the use case. A few small improvements:
- Mark props as immutable to satisfy Sonar and align with TS best practices:
-interface SunProps { - className?: string - variant?: 'regular' | 'solid' -} +interface SunProps { + readonly className?: string + readonly variant?: 'regular' | 'solid' +}
- Hoist the
pathsobject outside the component so it isn’t re‑allocated on every render:-interface SunProps { - readonly className?: string - readonly variant?: 'regular' | 'solid' -} - -export default function Sun({ className, variant = 'regular' }: SunProps) { - const paths = { +const SUN_PATHS = { solid: 'M 11 0 L 11 3 L 13 3 L 13 0 L 11 0 z ...', regular: 'M 11 0 L 11 3 L 13 3 L 13 0 L 11 0 z ...', - } +} as const + +interface SunProps { + readonly className?: string + readonly variant?: keyof typeof SUN_PATHS +} + +export default function Sun({ className, variant = 'regular' }: SunProps) {…and then:
- <path d={paths[variant]} /> + <path d={SUN_PATHS[variant]} />
- Since this icon is decorative inside a button that already has an
aria-label, consider hiding the SVG from assistive tech:- <svg + <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" className={className} fill="currentColor" + aria-hidden="true" + focusable="false" >
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/__tests__/unit/components/ModeToggle.test.tsx(1 hunks)frontend/jest.config.ts(1 hunks)frontend/src/components/ModeToggle.tsx(2 hunks)frontend/src/components/icons/Sun.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/tests/unit/components/ModeToggle.test.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T02:42:41.928Z
Learnt from: Rajgupta36
Repo: OWASP/Nest PR: 2288
File: frontend/src/components/ActionButton.tsx:0-0
Timestamp: 2025-09-17T02:42:41.928Z
Learning: In frontend/src/components/ActionButton.tsx, the user Rajgupta36 intentionally changed text-blue-600 to text-[#1D7BD7] to align the text color with the border color (#1D7BD7) for visual consistency, prioritizing design alignment over theme tokens.
Applied to files:
frontend/src/components/ModeToggle.tsx
🧬 Code graph analysis (1)
frontend/src/components/ModeToggle.tsx (1)
frontend/src/components/icons/Sun.tsx (1)
Sun(6-25)
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/components/icons/Sun.tsx
[warning] 6-6: Mark the props of the component as read-only.
🔇 Additional comments (2)
frontend/jest.config.ts (1)
5-12: Confirm intent to exclude all icon components from coverageAdding
'!src/components/icons/**'means none of the icon components (includingSun) will contribute to coverage metrics. If that’s deliberate (treating icons as purely presentational), this is fine; just double‑check there isn’t any logic in that folder you’d still want covered.frontend/src/components/ModeToggle.tsx (1)
7-40: Icon swap logic matches theme semantics and keeps UX consistentUsing the local
Sunicon whentheme === 'dark'and the moon icon otherwise aligns with the “show the target theme” convention, and the tooltip/aria-labelstrings remain consistent with that behavior. The change is nicely localized to the icon block without affecting the toggle logic.
|
@sameersharmadev I agree! After reviewing this PR I decided to revert the lighbulb and try something else to keep universal Sun/Moon design. @dhirukumar I know your original suggestion was to use lightbulb, but it is confusing to have a lightbulb and the moon. I tried to play around with changing style of the lightbulb for light/dark theme (on/off lightbulb) but did not really like it. So I decided to introduce custom icons via SVG. We will still rely on FA icons for most cases, but this should be a good fallback when we can not find suitable FA icon. I pushed some changes to this PR. Hope you don't mind! We'll still count it as completed by you since you've put a good effort in it. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/icons/Sun.tsx (1)
7-23: Hoistpathsand consider basic SVG a11y attributesTwo small optional improvements:
pathsis recreated on every render; it can be moved outside the component and markedas constto avoid reallocation and tighten typing.- If this icon is purely presentational inside a labeled button, consider hiding it from assistive tech via
aria-hidden(and optionallyfocusable="false").Example refactor:
-interface SunProps { - readonly className?: string - readonly variant?: 'regular' | 'solid' -} - -export default function Sun({ className, variant = 'regular' }: Readonly<SunProps>) { - const paths = { - solid: - 'M 11 0 L 11 3 L 13 3 L 13 0 L 11 0 z M 4.2226562 2.8085938 L 2.8085938 4.2226562 L 4.9296875 6.34375 L 6.34375 4.9296875 L 4.2226562 2.8085938 z M 19.777344 2.8085938 L 17.65625 4.9296875 L 19.070312 6.34375 L 21.191406 4.2226562 L 19.777344 2.8085938 z M 12 5 A 7 7 0 0 0 5 12 A 7 7 0 0 0 12 19 A 7 7 0 0 0 19 12 A 7 7 0 0 0 12 5 z M 0 11 L 0 13 L 3 13 L 3 11 L 0 11 z M 21 11 L 21 13 L 24 13 L 24 11 L 21 11 z M 4.9296875 17.65625 L 2.8085938 19.777344 L 4.2226562 21.191406 L 6.34375 19.070312 L 4.9296875 17.65625 z M 19.070312 17.65625 L 17.65625 19.070312 L 19.777344 21.191406 L 21.191406 19.777344 L 19.070312 17.65625 z M 11 21 L 11 24 L 13 24 L 13 21 L 11 21 z', - - regular: - 'M 11 0 L 11 3 L 13 3 L 13 0 L 11 0 z M 4.2226562 2.8085938 L 2.8085938 4.2226562 L 4.9296875 6.34375 L 6.34375 4.9296875 L 4.2226562 2.8085938 z M 19.777344 2.8085938 L 17.65625 4.9296875 L 19.070312 6.34375 L 21.191406 4.2226562 L 19.777344 2.8085938 z M 12 5 C 8.1458514 5 5 8.1458514 5 12 C 5 15.854149 8.1458514 19 12 19 C 15.854149 19 19 15.854149 19 12 C 19 8.1458514 15.854149 5 12 5 z M 12 7 C 14.773268 7 17 9.2267316 17 12 C 17 14.773268 14.773268 17 12 17 C 9.2267316 17 7 14.773268 7 12 C 7 9.2267316 9.2267316 7 12 7 z M 0 11 L 0 13 L 3 13 L 3 11 L 0 11 z M 21 11 L 21 13 L 24 13 L 24 11 L 21 11 z M 4.9296875 17.65625 L 2.8085938 19.777344 L 4.2226562 21.191406 L 6.34375 19.070312 L 4.9296875 17.65625 z M 19.070312 17.65625 L 17.65625 19.070312 L 19.777344 21.191406 L 21.191406 19.777344 L 19.070312 17.65625 z M 11 21 L 11 24 L 13 24 L 13 21 L 11 21 z', - } - - return ( - <svg - xmlns="http://www.w3.org/2000/svg" - viewBox="0 0 24 24" - className={className} - fill="currentColor" - > - <path d={paths[variant]} /> - </svg> - ) -} +interface SunProps { + readonly className?: string + readonly variant?: 'regular' | 'solid' +} + +const paths = { + solid: + 'M 11 0 L 11 3 L 13 3 L 13 0 L 11 0 z M 4.2226562 2.8085938 L 2.8085938 4.2226562 L 4.9296875 6.34375 L 6.34375 4.9296875 L 4.2226562 2.8085938 z M 19.777344 2.8085938 L 17.65625 4.9296875 L 19.070312 6.34375 L 21.191406 4.2226562 L 19.777344 2.8085938 z M 12 5 A 7 7 0 0 0 5 12 A 7 7 0 0 0 12 19 A 7 7 0 0 0 19 12 A 7 7 0 0 0 12 5 z M 0 11 L 0 13 L 3 13 L 3 11 L 0 11 z M 21 11 L 21 13 L 24 13 L 24 11 L 21 11 z M 4.9296875 17.65625 L 2.8085938 19.777344 L 4.2226562 21.191406 L 6.34375 19.070312 L 4.9296875 17.65625 z M 19.070312 17.65625 L 17.65625 19.070312 L 19.777344 21.191406 L 21.191406 19.777344 L 19.070312 17.65625 z M 11 21 L 11 24 L 13 24 L 13 21 L 11 21 z', + + regular: + 'M 11 0 L 11 3 L 13 3 L 13 0 L 11 0 z M 4.2226562 2.8085938 L 2.8085938 4.2226562 L 4.9296875 6.34375 L 6.34375 4.9296875 L 4.2226562 2.8085938 z M 19.777344 2.8085938 L 17.65625 4.9296875 L 19.070312 6.34375 L 21.191406 4.2226562 L 19.777344 2.8085938 z M 12 5 C 8.1458514 5 5 8.1458514 5 12 C 5 15.854149 8.1458514 19 12 19 C 15.854149 19 19 15.854149 19 12 C 19 8.1458514 15.854149 5 12 5 z M 12 7 C 14.773268 7 17 9.2267316 17 12 C 17 14.773268 14.773268 17 12 17 C 9.2267316 17 7 14.773268 7 12 C 7 9.2267316 9.2267316 7 12 7 z M 0 11 L 0 13 L 3 13 L 3 11 L 0 11 z M 21 11 L 21 13 L 24 13 L 24 11 L 21 11 z M 4.9296875 17.65625 L 2.8085938 19.777344 L 4.2226562 21.191406 L 6.34375 19.070312 L 4.9296875 17.65625 z M 19.070312 17.65625 L 17.65625 19.070312 L 19.777344 21.191406 L 21.191406 19.777344 L 19.070312 17.65625 z M 11 21 L 11 24 L 13 24 L 13 21 L 11 21 z', +} as const + +export default function Sun({ className, variant = 'regular' }: Readonly<SunProps>) { + return ( + <svg + xmlns="http://www.w3.org/2000/svg" + viewBox="0 0 24 24" + className={className} + fill="currentColor" + aria-hidden="true" + focusable="false" + > + <path d={paths[variant]} /> + </svg> + ) +}These are optional; feel free to keep the current implementation if it better matches existing patterns in this codebase.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/icons/Sun.tsx(1 hunks)
🔇 Additional comments (1)
frontend/src/components/icons/Sun.tsx (1)
1-25: Sun icon component looks correct and is nicely typedThe component is small, focused, and well-typed (
Readonly<SunProps>, defaultvariant = 'regular'). SVG usage andcurrentColormake it easy to style via CSS. No correctness issues from this file.
|
|
@kasya ,Thanks for the update! If you believe adding the custom SVG icons improves the consistency, then that’s great — I’m happy with it. I also prefer having a proper sun icon, so introducing external SVGs makes sense to me. |
3 participants
Description
This PR updates the Light Mode toggle icon in the UI.
What Was Changed
Replaced the old sun/gear-style icon (which looked like a settings icon):
Added the new light bulb icon (more intuitive for Light Mode):
before
After
🧠 Why This Change
The previous icon was visually misleading and confused users as it closely resembled the settings icon.
The new icon provides clearer UX and better represents Light Mode functionality.
🧪 Testing
Verified UI behavior in both Light and Dark modes
Ensured toggle works with no functional regressions
📝 Checklist
I've read and followed the contributing guidelines
I've run make check-test locally; all checks and tests passed.
Closes #2713