feat: add dark-light mode support for default theme#1120
feat: add dark-light mode support for default theme#1120Robert27 wants to merge 3 commits intosysadminsmedia:mainfrom
Conversation
WalkthroughThis PR introduces dark mode support for the homebox theme by extending the theme composable with a DarkModePreference type and setDarkMode function, creating a new DarkModeToggle component, updating CSS variables for light/dark color schemes, and integrating the toggle into the sidebar footer. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–25 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Security Recommendations
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
Robert27
changed the title
![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/public/set-theme.js (1)
3-6: Consider validating the preferences structure and theme value.The parsed
preferencesobject could be malformed or tampered with in localStorage. While this is a low-risk scenario (self-XSS), validating the structure adds robustness:
preferences.themecould be any string, not just validDaisyThemevaluespreferences.darkModefalls back to"auto"which is goodFor defense in depth, consider validating that
themeis one of the expected values before applying it to the DOM.const preferences = JSON.parse(localStorage.getItem("homebox/preferences/location")); if (preferences) { const theme = preferences.theme; const darkMode = preferences.darkMode || "auto"; + + // Basic validation - only apply known themes + const validThemes = ["homebox", "light", "dark", "cupcake", "bumblebee", "emerald", "corporate", "synthwave", "retro", "cyberpunk", "valentine", "halloween", "garden", "forest", "aqua", "lofi", "pastel", "fantasy", "wireframe", "black", "luxury", "dracula", "cmyk", "autumn", "business", "acid", "lemonade", "night", "coffee", "winter"]; + if (!validThemes.includes(theme)) { + console.warn("Invalid theme in preferences:", theme); + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
frontend/app.vue(1 hunks)frontend/assets/css/main.css(1 hunks)frontend/components/App/DarkModeToggle.vue(1 hunks)frontend/components/ui/sidebar/Sidebar.vue(2 hunks)frontend/composables/use-preferences.ts(2 hunks)frontend/composables/use-theme.ts(3 hunks)frontend/layouts/default.vue(2 hunks)frontend/locales/en.json(1 hunks)frontend/public/set-theme.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,vue}
⚙️ CodeRabbit configuration file
**/*.{ts,vue}: Check for hardcoded strings in UI components that should be translatable.
Look for:
- String literals in Vue components (e.g. Click me)
- Alert messages, error messages, and user-facing text
- Placeholder text and labels
Files:
frontend/composables/use-preferences.tsfrontend/layouts/default.vuefrontend/components/App/DarkModeToggle.vuefrontend/app.vuefrontend/composables/use-theme.tsfrontend/components/ui/sidebar/Sidebar.vue
🧬 Code graph analysis (1)
frontend/composables/use-preferences.ts (1)
frontend/lib/data/themes.ts (1)
DaisyTheme(1-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Backend Server Tests / Go
- GitHub Check: build (linux/arm/v7)
- GitHub Check: build (linux/amd64)
- GitHub Check: build (linux/arm64)
🔇 Additional comments (19)
frontend/composables/use-preferences.ts (2)
14-15: LGTM! Clean type definition and preference integration.The
DarkModePreferencetype is well-defined with appropriate literal union values. The integration intoLocationViewPreferencesfollows the existing pattern.Also applies to: 22-22
52-52: Good default choice.Defaulting to
"auto"respects user system preferences and provides the expected behavior out of the box.frontend/locales/en.json (1)
612-615: LGTM! Translation keys properly added.The keys follow the existing
menu.*naming convention. As noted in the PR, these keys need to be added to other language files for complete i18n support.frontend/composables/use-theme.ts (5)
19-21: Good use of VueUse's reactive media query.The
currentThemeClasstracking pattern prevents CSS class accumulation when switching themes.
23-47: Clean DOM manipulation with proper class tracking.The function correctly manages theme classes by tracking the current class and removing it before applying the new one, preventing class accumulation.
60-90: LGTM! Dark mode logic is well-structured.The function correctly:
- Restricts dark mode to the homebox theme
- Handles all three preference modes (auto/light/dark)
- Syncs with system preference when in auto mode
113-124: Well-scoped watchers for preference changes.The system dark mode watcher correctly only triggers
applyDarkModewhen the preference is"auto", avoiding unnecessary DOM updates.
126-134: Guard prevents redundant theme applications.The check
themeRef.value !== newThemeavoids unnecessary DOM updates when the theme is already applied.frontend/public/set-theme.js (2)
19-40: Logic duplication with use-theme.ts - keep in sync.The dark mode logic here mirrors
applyDarkModeinuse-theme.ts. This duplication is acceptable for FOUC prevention, but changes to one must be reflected in the other to maintain consistency.
43-45: Appropriate error handling for a pre-hydration script.Silent failure with console logging is correct here - theme initialization should not break the application.
frontend/app.vue (1)
8-8: Potential duplication of homebox class management in template binding.The
:classbinding on line 8 that conditionally applies thehomeboxclass may duplicate logic fromapplyThemeToDOMin use-theme.ts (lines 39-43). If both mechanisms manage the same class, this could introduce race conditions during hydration or cause redundant class operations.Verify whether:
- The template binding is necessary for SSR initial render correctness
applyThemeToDOMalone is sufficient without the template-level class bindingfrontend/layouts/default.vue (2)
93-113: LGTM! Clean integration of dark mode toggle.The conditional rendering of the DarkModeToggle component is properly scoped to the homebox theme, and the refactored footer structure maintains consistency with the existing sidebar component patterns. All user-facing strings are properly internationalized using
$t().
238-238: LGTM! Import statement is correct.The DarkModeToggle component is properly imported and used in the template.
frontend/assets/css/main.css (2)
8-37: LGTM! Updated homebox light mode colors.The adjusted color values provide a brighter, lighter appearance for the homebox theme in light mode. The changes maintain consistency across all color tokens.
39-72: LGTM! Well-structured dark mode variant.The dark mode implementation is properly scoped to the homebox theme using dual selectors (
data-dark-mode="dark"and.darkclass). The color palette uses neutral, darker tones with appropriate contrast ratios. The comprehensive set of CSS custom properties ensures all UI elements are styled correctly in dark mode.frontend/components/App/DarkModeToggle.vue (2)
1-49: LGTM! Excellent internationalization and type safety.The component properly uses
useI18nand defines all user-facing labels as translation functions. The use of TypeScript types (DarkModePreference,Component) ensures type safety, and the computedcurrentOptionwith optional chaining provides defensive fallback handling.
51-72: LGTM! Template correctly uses translations and reactive values.All user-facing text is properly internationalized using
$t()and the label functions. The active option highlighting (line 64) correctly compares the reactivedarkModevalue, which auto-unwraps in Vue 3 templates.frontend/components/ui/sidebar/Sidebar.vue (2)
34-34: LGTM! Class reordering for consistency.The classes have been reordered without any functional changes. This maintains the same styling while improving code organization.
82-84: LGTM! Added visual separation with border.The addition of
border-r border-sidebar-borderprovides the right border for visual separation mentioned in the PR objectives. The styling properly uses the CSS custom property for theme consistency.
|
I pulled this and tested locally. LGTM, didn't see any issues and love this feature. Would be great to get it merged in! |
|
Currently we are avoiding merging non bug fix changes until we get the 0.22 release done (so as to try to prevent us from introducing new bugs) |
Awesome, sounds good. Thanks for your work on Homebox, really appreciate it! |
|
Sorry for the delay in getting to this. Overall I do quite like the dark design and it makes sense to me, my hesitation with merging this is if we do overhaul the themes system (which is something I think very much needs doing) then we may be adding more stuff that will either need backwards compat or people will be annoyed is missing. |
I tend to agree with @tonyaellie here. Thoughts please @tankerkiller125 ? |
Unfortunate, but understandable for sure. Thanks for taking the time to review. |
4 participants
What type of PR is this?
What this PR does / why we need it:
This PR adds dark/light mode support to the homebox theme with a 3-way toggle (System, Light, Dark) in the sidebar footer.
Changes made:
frontend/composables/use-theme.ts: Fixed FIXME by implementing proper theme class tracking (removed inefficient hack)frontend/components/App/DarkModeToggle.vue: Added localized 3-way toggle with "System" label, fixed button text to show current selectionfrontend/layouts/default.vue: Moved toggle to sidebar footer, only shows for homebox themefrontend/assets/css/main.css: Updated homebox light mode (brighter background, lighter boxes) and dark mode (darker, neutral colors)frontend/components/ui/sidebar/Sidebar.vue: Added right border for better visual separationfrontend/public/set-theme.js: Updated to properly handle homebox theme dark modefrontend/locales/en.json: Added translation keys for dark mode optionsWhich issue(s) this PR fixes:
See #1117
Special notes for your reviewer:
Testing
(fill-in or delete this section)
Manual testing:
Code verification:
Summary by CodeRabbit
Release Notes
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.