[DT-2854] move dark mode button from side nav to top nav #2720
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
|
beeb50b to
d52a29b
Compare
| useDarkModePreference, | ||
| } from './dark-mode'; | ||
|
|
||
| describe('dark-mode utilities', () => { |
There was a problem hiding this comment.
Annotation: Thanks AI.
| <IconButton | ||
| variant="ghost" | ||
| label={buttonText} | ||
| icon={$useDarkMode ? 'moon' : 'sun'} |
There was a problem hiding this comment.
I think we'll want a third icon here, right?
Same for dark-mode-navigation-button.svelte
There was a problem hiding this comment.
Aha. Could you link me to that node in Figma? I didn't see this part....
I'll also loop in @bilal-karim on the PR.
There was a problem hiding this comment.
There was a problem hiding this comment.
Thanks for the 8000 catch @rossedfort.
Hey @andrewzamojc — sorry, I didn't make it clear in the designs or the ticket. The icon placement is correct in the top nav, but the icons should be:
- Light mode: https://holocene.preview.thundergun.io/?path=/story/icon--default&args=name:sun
- Dark mode: https://holocene.preview.thundergun.io/?path=/story/icon--default&args=name:moon
- System pref: It looks like this one doesn't exist in Holocene, but did exist in Figma. I'm attaching the SVG below so it can be added.
There was a problem hiding this comment.
This is done. Dark mode buttons now use system-window icon.
src/lib/components/side-nav.svelte
Outdated
| /> | ||
| <slot name="bottom" /> | ||
| </svelte:fragment> | ||
| <slot name="bottom" /> |
There was a problem hiding this comment.
I think we'll need the below. This is definitely tricky and will help when we move to Svelte5 (don't need that for this PR). Basically there's multiple slots that we need to pass down through.
<svelte:fragment slot="bottom" name="bottom" >
or
<svelte:fragment slot="bottom">
<slot name="bottom">
</svelte:fragment>
| useDarkModePreference, | ||
| } from '$lib/utilities/dark-mode'; | ||
|
|
||
| let buttonText = $derived( |
There was a problem hiding this comment.
Tiniest of nits, but usually if you are using $derived it should be a const. You can update $derived but it's only its for optimistic updates or some other edge case
There was a problem hiding this comment.
Good catch. I don't know why I used let here. I'll update it before merging.
* add useDarkModePreference store, make existing useDarkMode derived * add dark mode button component * add dark mode button to top nav * add DarkModeButton to bottom nav * remove dark mode button from side nav * enhance dark mode utilities with preference type and next preference function * separate icon button and nav button for bottom nav * add some tests for the dark-mode file * use original default value from .env * use svelte5, tidy up * add system-window icon * use system-window icon for dark mode buttons * tweak side nav slot "bottom" to be more like original * change let to const in dark-mode buttons
Description & motivation 💭
This PR enhances the app's dark mode functionality by adding a "System Default" option. This allows users to set their app's light/dark mode to defer to their system setting.
useDarkModestore is now derived from the preference, and continues to always return true/false. It resolves the system case to a boolean. This way existing code keeps working.Screenshots (if applicable) 📸
Design Considerations 🎨
Testing 🧪
How was this tested 👻
Manual testing, plus added a unit test.
Made preview PR on cloud-ui and manually tested there as well, https://github.com/temporalio/cloud-ui/pull/1460.
Steps for others to test: 🚶🏽♂️🚶🏽♀️
To see the change:
Checklists
Draft Checklist
Merge Checklist
Issue(s) closed
Docs
Any docs updates needed?