Skip to content

Commit 3e396ca

Browse files
raifdmuellerclaude
andcommitted
fix: Address CodeRabbit review comments on onboarding modal
- Add try/catch around localStorage in shouldShowOnboarding() and closeOnboarding() - Use window.matchMedia for conditional iframe rendering (no iframe on mobile) - Add focus trap (Tab/Shift+Tab) in modal keydown handler - Fix i18n key names in design doc (slogan1/slogan2) - Add matchMedia mock in tests for jsdom compatibility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 44f807a commit 3e396ca

3 files changed

Lines changed: 70 additions & 7 deletions

File tree

docs/plans/2026-03-08-onboarding-modal-design.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ Info icon (i) button next to site title. Calls `showOnboarding()`.
2929

3030
### 4. i18n Keys
3131

32-
New keys in en.json/de.json: `onboarding.slogan`, `onboarding.text1`-`text4`, `onboarding.cta`, `onboarding.watchVideo`, `onboarding.infoButton`
32+
New keys in en.json/de.json: `onboarding.slogan1`, `onboarding.slogan2`, `onboarding.text1`-`text4`, `onboarding.cta`, `onboarding.watchVideo`, `onboarding.infoButton`
3333

3434
### 5. Videos
3535

website/src/components/onboarding-modal.js

Lines changed: 41 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,11 @@ const YOUTUBE_VIDEOS = {
77
}
88

99
export function shouldShowOnboarding() {
10-
return localStorage.getItem(STORAGE_KEY) !== 'true'
10+
try {
11+
return localStorage.getItem(STORAGE_KEY) !== 'true'
12+
} catch {
13+
return false
14+
}
1115
}
1216

1317
export function createOnboardingModal() {
@@ -25,8 +29,33 @@ export function createOnboardingModal() {
2529
})
2630

2731
document.addEventListener('keydown', (e) => {
28-
if (e.key === 'Escape' && !modal.classList.contains('hidden')) {
32+
if (modal.classList.contains('hidden')) return
33+
34+
if (e.key === 'Escape') {
2935
closeOnboarding()
36+
return
37+
}
38+
39+
if (e.key === 'Tab') {
40+
const focusable = modal.querySelectorAll(
41+
'button, [href], input, select, textarea, iframe, [tabindex]:not([tabindex="-1"])'
42+
)
43+
if (focusable.length === 0) return
44+
45+
const first = focusable[0]
46+
const last = focusable[focusable.length - 1]
47+
48+
if (e.shiftKey) {
49+
if (document.activeElement === first) {
50+
e.preventDefault()
51+
last.focus()
52+
}
53+
} else {
54+
if (document.activeElement === last) {
55+
e.preventDefault()
56+
first.focus()
57+
}
58+
}
3059
}
3160
})
3261

@@ -82,7 +111,8 @@ function buildModalContent() {
82111
<div class="px-6 pb-4">
83112
<div class="flex flex-col sm:flex-row gap-6">
84113
<div class="sm:w-1/2 flex-shrink-0 flex items-center justify-center">
85-
<div class="hidden sm:block rounded-xl overflow-hidden bg-black aspect-[9/16] max-h-[400px] w-full">
114+
${window.matchMedia('(min-width: 640px)').matches ? `
115+
<div class="rounded-xl overflow-hidden bg-black aspect-[9/16] max-h-[400px] w-full">
86116
<iframe
87117
src="${embedUrl}"
88118
title="${watchVideo}"
@@ -92,17 +122,19 @@ function buildModalContent() {
92122
allowfullscreen
93123
></iframe>
94124
</div>
125+
` : `
95126
<a
96127
href="${youtubeUrl}"
97128
target="_blank"
98129
rel="noopener noreferrer"
99-
class="sm:hidden flex items-center gap-2 text-[var(--color-primary)] hover:underline font-medium py-2"
130+
class="flex items-center gap-2 text-[var(--color-primary)] hover:underline font-medium py-2"
100131
>
101132
<svg class="h-6 w-6" fill="currentColor" viewBox="0 0 24 24">
102133
<path d="M23.498 6.186a3.016 3.016 0 00-2.122-2.136C19.505 3.545 12 3.545 12 3.545s-7.505 0-9.377.505A3.017 3.017 0 00.502 6.186C0 8.07 0 12 0 12s0 3.93.502 5.814a3.016 3.016 0 002.122 2.136c1.871.505 9.376.505 9.376.505s7.505 0 9.377-.505a3.015 3.015 0 002.122-2.136C24 15.93 24 12 24 12s0-3.93-.502-5.814zM9.545 15.568V8.432L15.818 12l-6.273 3.568z"/>
103134
</svg>
104135
${watchVideo}
105136
</a>
137+
`}
106138
</div>
107139
108140
<div class="sm:w-1/2 flex flex-col gap-3 text-[var(--color-text)] text-sm leading-relaxed">
@@ -148,5 +180,9 @@ export function closeOnboarding() {
148180
modal.classList.add('hidden')
149181
modal.classList.remove('flex')
150182
document.body.style.overflow = ''
151-
localStorage.setItem(STORAGE_KEY, 'true')
183+
try {
184+
localStorage.setItem(STORAGE_KEY, 'true')
185+
} catch {
186+
// localStorage may be blocked (private browsing, security settings)
187+
}
152188
}

website/src/components/onboarding-modal.test.js

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,32 @@ import {
88
} from './onboarding-modal.js'
99

1010
describe('onboarding-modal', () => {
11+
let originalMatchMedia
12+
1113
beforeEach(() => {
1214
localStorage.clear()
1315
i18n.init()
1416
// Note: innerHTML usage here is in test code only, not production.
1517
// The test DOM is reset between tests and contains no user input.
1618
document.body.innerHTML = ''
19+
// Mock matchMedia - default to desktop (min-width: 640px matches)
20+
originalMatchMedia = window.matchMedia
21+
window.matchMedia = (query) => ({
22+
matches: query === '(min-width: 640px)',
23+
media: query,
24+
onchange: null,
25+
addListener: () => {},
26+
removeListener: () => {},
27+
addEventListener: () => {},
28+
removeEventListener: () => {},
29+
dispatchEvent: () => false,
30+
})
1731
})
1832

1933
afterEach(() => {
2034
document.body.innerHTML = ''
2135
document.body.style.overflow = ''
36+
window.matchMedia = originalMatchMedia
2237
})
2338

2439
describe('shouldShowOnboarding', () => {
@@ -86,13 +101,25 @@ describe('onboarding-modal', () => {
86101
expect(iframe.src).toContain('youtube.com/embed/cp-qqiHU-MA')
87102
})
88103

89-
it('contains the mobile YouTube link', () => {
104+
it('contains YouTube link on mobile instead of iframe', () => {
105+
window.matchMedia = (query) => ({
106+
matches: false,
107+
media: query,
108+
onchange: null,
109+
addListener: () => {},
110+
removeListener: () => {},
111+
addEventListener: () => {},
112+
removeEventListener: () => {},
113+
dispatchEvent: () => false,
114+
})
90115
createOnboardingModal()
91116
showOnboarding()
92117

93118
const link = document.querySelector('#onboarding-modal a[href*="youtube.com/shorts"]')
94119
expect(link).toBeTruthy()
95120
expect(link.getAttribute('target')).toBe('_blank')
121+
const iframe = document.querySelector('#onboarding-modal iframe')
122+
expect(iframe).toBeNull()
96123
})
97124
})
98125

0 commit comments

Comments
 (0)