Skip to content

Commit 9577e1a

Browse files
Merge pull request #294 from OneBusAway/fix-292
feat: replace dark mode toggle with system preference detection
2 parents f7f170d + 508c3e7 commit 9577e1a

File tree

6 files changed

+53
-126
lines changed

6 files changed

+53
-126
lines changed

src/components/navigation/Header.svelte

Lines changed: 19 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@
66
} from '$env/static/public';
77
88
import { onMount } from 'svelte';
9-
import ThemeSwitcher from '$lib/ThemeSwitch/ThemeSwitcher.svelte';
109
import MobileMenu from './MobileMenu.svelte';
1110
1211
const showRegionName = __SHOW_REGION_NAME_IN_NAV_BAR__;
@@ -17,7 +16,6 @@
1716
let linksContainer = $state(null);
1817
let cachedLinksWidth = 0;
1918
20-
const THEME_SWITCHER_WIDTH = 56; // Width of the theme switcher component in pixels
2119
const EXTRA_PADDING = 32; // Extra padding to account for padding and margins in pixels
2220
const LINKS_GAP = '1rem'; // Matches gap-x-4 in the template
2321
const LINK_PADDING = '0.25rem 0.5rem'; // Matches px-2 py-1 in the template
@@ -37,7 +35,7 @@
3735
3836
const navWidth = navContainer.clientWidth;
3937
const logoWidth = navContainer.querySelector('.logo-container')?.clientWidth || 0;
40-
const availableWidth = navWidth - logoWidth - THEME_SWITCHER_WIDTH - EXTRA_PADDING;
38+
const availableWidth = navWidth - logoWidth - EXTRA_PADDING;
4139
4240
let linksWidth = 0;
4341
if (linksContainer) {
@@ -137,30 +135,24 @@
137135
</div>
138136
{/if}
139137
140-
<div class="flex items-center">
141-
{#if shouldShowMobile}
142-
<button onclick={toggleNavbar} aria-label="Toggle navigation menu" class="mr-2">
143-
<svg
144-
class="burger-icon h-6 w-6 text-brand-foreground dark:text-surface-foreground-dark"
145-
fill="none"
146-
stroke="currentColor"
147-
viewBox="0 0 24 24"
148-
xmlns="http://www.w3.org/2000/svg"
149-
>
150-
<path
151-
stroke-linecap="round"
152-
stroke-linejoin="round"
153-
stroke-width="2"
154-
d="M4 6h16M4 12h16m-7 6h7"
155-
></path>
156-
</svg>
157-
</button>
158-
{:else}
159-
<div class={shouldShowMobile ? '' : 'flex'}>
160-
<ThemeSwitcher />
161-
</div>
162-
{/if}
163-
</div>
138+
{#if shouldShowMobile}
139+
<button onclick={toggleNavbar} aria-label="Toggle navigation menu" class="mr-2">
140+
<svg
141+
class="burger-icon h-6 w-6 text-brand-foreground dark:text-surface-foreground-dark"
142+
fill="none"
143+
stroke="currentColor"
144+
viewBox="0 0 24 24"
145+
xmlns="http://www.w3.org/2000/svg"
146+
>
147+
<path
148+
stroke-linecap="round"
149+
stroke-linejoin="round"
150+
stroke-width="2"
151+
d="M4 6h16M4 12h16m-7 6h7"
152+
></path>
153+
</svg>
154+
</button>
155+
{/if}
164156
</div>
165157
166158
{#if isMobileMenuOpen}

src/components/navigation/MobileMenu.svelte

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
<script>
22
import { fly } from 'svelte/transition';
3-
import ThemeSwitcher from '$lib/ThemeSwitch/ThemeSwitcher.svelte';
43
54
let { headerLinks = {}, closeMenu } = $props();
65
</script>
@@ -31,10 +30,6 @@
3130
>
3231
{/each}
3332
</div>
34-
35-
<div>
36-
<ThemeSwitcher />
37-
</div>
3833
</div>
3934

4035
<style lang="postcss">

src/components/navigation/__tests__/MobileMenu.test.js

Lines changed: 4 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -96,19 +96,6 @@ describe('MobileMenu', () => {
9696
expect(closeButton).toHaveAttribute('aria-label', 'Close Menu');
9797
});
9898

99-
test('renders theme switcher component', () => {
100-
render(MobileMenu, {
101-
props: {
102-
headerLinks: mockHeaderLinks,
103-
closeMenu: mockCloseMenu
104-
}
105-
});
106-
107-
// Theme switcher should be present (it's a checkbox with sr-only class)
108-
const themeToggle = screen.getByRole('checkbox', { name: 'Toggle theme' });
109-
expect(themeToggle).toBeInTheDocument();
110-
});
111-
11299
test('handles empty header links gracefully', () => {
113100
render(MobileMenu, {
114101
props: {
@@ -117,9 +104,8 @@ describe('MobileMenu', () => {
117104
}
118105
});
119106

120-
// Should still render close button and theme switcher
107+
// Should still render close button
121108
expect(screen.getByRole('button', { name: 'Close Menu' })).toBeInTheDocument();
122-
expect(screen.getByRole('checkbox', { name: 'Toggle theme' })).toBeInTheDocument();
123109
});
124110

125111
test('applies correct CSS classes for full-screen overlay', () => {
@@ -263,7 +249,6 @@ describe('MobileMenu', () => {
263249

264250
// Should render with empty headerLinks object
265251
expect(screen.getByRole('button', { name: 'Close Menu' })).toBeInTheDocument();
266-
expect(screen.getByRole('checkbox', { name: 'Toggle theme' })).toBeInTheDocument();
267252
});
268253

269254
test('accessibility: close button can be focused', async () => {
@@ -298,40 +283,22 @@ describe('MobileMenu', () => {
298283
expect(homeLink).toHaveFocus();
299284
});
300285

301-
test('accessibility: theme switcher can be focused', async () => {
302-
render(MobileMenu, {
303-
props: {
304-
headerLinks: mockHeaderLinks,
305-
closeMenu: mockCloseMenu
306-
}
307-
});
308-
309-
const themeToggle = screen.getByRole('checkbox', { name: 'Toggle theme' });
310-
311-
// Focus the theme toggle
312-
themeToggle.focus();
313-
expect(themeToggle).toHaveFocus();
314-
});
315-
316-
test('renders in correct order: close button, links, theme switcher', () => {
286+
test('renders in correct order: close button, then links', () => {
317287
const { container } = render(MobileMenu, {
318288
props: {
319289
headerLinks: mockHeaderLinks,
320290
closeMenu: mockCloseMenu
321291
}
322292
});
323293

324-
const elements = Array.from(container.querySelectorAll('button, a, input'));
294+
const elements = Array.from(container.querySelectorAll('button, a'));
325295

326296
// First element should be the close button
327297
expect(elements[0]).toHaveAttribute('aria-label', 'Close Menu');
328298

329-
// Middle elements should be navigation links
299+
// Rest should be navigation links
330300
expect(elements[1]).toHaveAttribute('href', '/');
331301
expect(elements[2]).toHaveAttribute('href', '/about');
332-
333-
// Last element should be theme toggle
334-
expect(elements[elements.length - 1]).toHaveAttribute('aria-label', 'Toggle theme');
335302
});
336303

337304
test('handles single navigation link', () => {

src/lib/ThemeSwitch/ThemeSwitcher.svelte

Lines changed: 0 additions & 57 deletions
This file was deleted.

src/lib/systemTheme.js

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
import { browser } from '$app/environment';
2+
3+
/**
4+
* Initializes the system theme listener.
5+
* Detects the user's system preference for dark/light mode and applies it.
6+
* Listens for real-time changes to system preferences.
7+
*/
8+
export function initSystemTheme() {
9+
if (!browser) return;
10+
11+
const mediaQuery = window.matchMedia('(prefers-color-scheme: dark)');
12+
13+
function applyTheme(isDark) {
14+
document.documentElement.classList.toggle('dark', isDark);
15+
// Dispatch event for map providers
16+
window.dispatchEvent(new CustomEvent('themeChange', { detail: { darkMode: isDark } }));
17+
}
18+
19+
// Apply initial theme
20+
applyTheme(mediaQuery.matches);
21+
22+
// Listen for system preference changes
23+
mediaQuery.addEventListener('change', (e) => applyTheme(e.matches));
24+
25+
// Clean up old localStorage preference
26+
localStorage.removeItem('theme');
27+
}

src/routes/+layout.svelte

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import { locale } from 'svelte-i18n';
88
import { onMount } from 'svelte';
99
import analytics from '$lib/Analytics/PlausibleAnalytics.js';
10+
import { initSystemTheme } from '$lib/systemTheme.js';
1011
import { env } from '$env/dynamic/public';
1112
1213
const faviconUrl = env.FAVICON_URL || '/favicon-32x32.png';
@@ -21,6 +22,8 @@
2122
config.autoAddCss = false; // Tell Font Awesome to skip adding the CSS automatically since it's being imported above
2223
2324
onMount(() => {
25+
initSystemTheme();
26+
2427
locale.subscribe((lang) => {
2528
if (lang === 'ar') {
2629
document.documentElement.classList.add('rtl');

0 commit comments

Comments
 (0)