feat: Enhanced theme switcher with system preference detection and tri-state selection #4639
Conversation
… theme management - Added FullScreenThemeSelector component for theme selection with icons. - Introduced useTheme hook to manage theme state and preferences. - Replaced previous dark theme switcher with a dropdown for theme selection in HeaderTools and Example components. - Updated SideNavLayout to utilize the new theme management system.
There was a problem hiding thi 8000 s comment.
I think you and your robot friend did a good job. I read through and it seems understandable. I just left a few nitpicky comments about the switches, but you're free to ignore. It won't cause any problems if it's left as-is.
Gotta figure out my back-up plan for when our robot overlords put me out of work. :)
packages/documentation-framework/layouts/sideNavLayout/sideNavLayout.js
Outdated
Show resolved
Hide resolved
packages/documentation-framework/layouts/sideNavLayout/sideNavLayout.js
Outdated
Show resolved
Hide resolved
|
Can we force the dropdown to show a generic name rather than system? @edonehoo any thoughts on verbiage? |
|
I would recommend "System theme", "Light theme", "Dark theme" as the toggle text to reflect what's selected or pre-selected Or you could just have "Theme" and not show selections in the toggle (like Atlassian does) |
|
Theme may get confusing as we have a distinction between themes and modes. A new Red Hat theme would have light/dark modes. |
There was a problem hiding this comment.
In addition to the comment below, one non-blocker I'm noticing is when the page first loads, the "System" default can be seen before updating to the theme selection in local storage (at least when "Light" is the stored theme). It happens really quickly and probably only noticeable if you're looking for it, but if it is something we'd want to take care of a followup issue would be fine (but also if we want to see if astro will handle it we can do that sa well).
packages/documentation-framework/layouts/sideNavLayout/sideNavLayout.js
Outdated
Show resolved
Hide resolved
|
Good point, could say "Display mode" instead? If we want to move away from light/dark "theme" and towards "mode", it would be good to reflect that in the code (rather than the vars/methods like useTheme getStoredThemeMode) and the docs consistently. Otherwise, it feels like we're scrambling the language we're using with devs vs designers (and so on, etc) |
🎨 Enhanced Theme Switcher Implementation
This PR implements a comprehensive theme switcher that provides users with three choices: System, Light, and Dark themes, with proper persistence and synchronization across the entire application.
✨ Features
🖥️ Automatic System Preference Detection
prefers-color-schemesetting automatically🎯 Tri-State Theme Selection
💾 Persistent User Preferences
🔄 Perfect Synchronization
🛠️ Implementation Details
New Shared Theme Hook (
useTheme.js)Enhanced UI Components
Architecture Improvements
SideNavLayoutlevel for single source of truth🐛 Bug Fixes
🎭 User Experience
📦 Files Changed
packages/documentation-framework/hooks/useTheme.js(new)packages/documentation-framework/layouts/sideNavLayout/sideNavLayout.jspackages/documentation-framework/components/example/example.js🧪 Testing
This implementation has been tested with:
🎯 Addresses
This implementation builds upon and completes the work started in PR #4367, providing the full tri-state theme selection that was originally requested while fixing the synchronization issues that prevented the earlier implementation from being merged.
Ready for review! 🚀