Skip to content

Commit 9921153

Browse files
committed
Fix dialog centering, revert to scrollable image layout
Root cause: global reset (* { margin: 0 }) overrides the UA stylesheet's margin: auto that centers <dialog>. Fix by restoring margin: auto on .screenshot-dialog. Also reverts prev/next navigation back to the simpler scrollable layout with all images stacked vertically.
1 parent d0c5978 commit 9921153

4 files changed

Lines changed: 13 additions & 101 deletions

File tree

src/design/components/lab-card/index.tsx

Lines changed: 4 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -90,34 +90,19 @@ export function LabCard({ lab, basePath = '/' }: { lab: FeaturedLab; basePath?:
9090
{screenshotLinks.map(({ link, id }) => (
9191
<screenshot-dialog>
9292
<dialog id={id} class="screenshot-dialog">
93-
<div class="screenshot-dialog__header">
94-
<span class="screenshot-dialog__counter" aria-live="polite"></span>
95-
<button class="screenshot-dialog__close" data-close-dialog aria-label="Close" type="button">
96-
&times;
97-
</button>
98-
</div>
93+
<button class="screenshot-dialog__close" data-close-dialog aria-label="Close" type="button">
94+
&times;
95+
</button>
9996
<div class="screenshot-dialog__images">
100-
{link.images.map((img, i) => (
97+
{link.images.map((img) => (
10198
<img
10299
class="screenshot-dialog__img"
103100
src={url(img.src, basePath)}
104101
alt={img.alt}
105102
loading="lazy"
106-
data-index={i}
107-
hidden={i > 0}
108103
/>
109104
))}
110105
</div>
111-
{link.images.length > 1 ? (
112-
<div class="screenshot-dialog__nav">
113-
<button class="screenshot-dialog__nav-btn" data-prev type="button" aria-label="Previous screenshot">
114-
&larr; Prev
115-
</button>
116-
<button class="screenshot-dialog__nav-btn" data-next type="button" aria-label="Next screenshot">
117-
Next &rarr;
118-
</button>
119-
</div>
120-
) : null}
121106
</dialog>
122107
</screenshot-dialog>
123108
))}

src/design/components/lab-card/styles.css

Lines changed: 7 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -121,29 +121,22 @@
121121
.screenshot-dialog {
122122
max-inline-size: min(900px, 90vw);
123123
max-block-size: 90vh;
124+
margin: auto; /* restore centering lost by global * { margin: 0 } reset */
124125
border: none;
125126
border-radius: var(--radius-sm);
126127
padding: var(--space-6);
127128
overflow-y: auto;
129+
position: relative;
128130
}
129131

130132
.screenshot-dialog::backdrop {
131133
background: rgb(0 0 0 / 0.6);
132134
}
133135

134-
.screenshot-dialog__header {
135-
display: flex;
136-
justify-content: space-between;
137-
align-items: center;
138-
margin-block-end: var(--space-4);
139-
}
140-
141-
.screenshot-dialog__counter {
142-
font-size: var(--step--1);
143-
color: var(--color-ink-subtle);
144-
}
145-
146136
.screenshot-dialog__close {
137+
position: absolute;
138+
inset-block-start: var(--space-3);
139+
inset-inline-end: var(--space-3);
147140
background: none;
148141
border: none;
149142
font-size: var(--step-3);
@@ -152,7 +145,6 @@
152145
color: var(--color-ink-subtle);
153146
padding: var(--space-1) var(--space-2);
154147
border-radius: var(--radius-sm);
155-
margin-inline-start: auto;
156148
}
157149

158150
.screenshot-dialog__close:hover,
@@ -163,7 +155,8 @@
163155

164156
.screenshot-dialog__images {
165157
display: flex;
166-
justify-content: center;
158+
flex-direction: column;
159+
gap: var(--space-4);
167160
}
168161

169162
.screenshot-dialog__img {
@@ -172,32 +165,4 @@
172165
border-radius: var(--radius-sm);
173166
box-shadow: var(--shadow-card);
174167
}
175-
176-
.screenshot-dialog__nav {
177-
display: flex;
178-
justify-content: space-between;
179-
margin-block-start: var(--space-4);
180-
}
181-
182-
.screenshot-dialog__nav-btn {
183-
background: none;
184-
border: 1px solid var(--color-surface-alt);
185-
border-radius: var(--radius-sm);
186-
padding: var(--space-2) var(--space-4);
187-
cursor: pointer;
188-
font: inherit;
189-
font-size: var(--step--1);
190-
color: var(--color-link);
191-
}
192-
193-
.screenshot-dialog__nav-btn:hover,
194-
.screenshot-dialog__nav-btn:focus-visible {
195-
background: var(--color-surface-alt);
196-
color: var(--color-link-hover);
197-
}
198-
199-
.screenshot-dialog__nav-btn:disabled {
200-
opacity: 0.4;
201-
cursor: default;
202-
}
203168
}

src/design/components/screenshot-dialog/client.ts

Lines changed: 1 addition & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -1,59 +1,27 @@
11
class ScreenshotDialogElement extends HTMLElement {
22
private dialog: HTMLDialogElement | null = null
3-
private images: HTMLImageElement[] = []
4-
private current = 0
53

64
connectedCallback() {
75
this.dialog = this.querySelector('dialog')
86
if (!this.dialog) return
97

10-
this.images = Array.from(this.dialog.querySelectorAll('.screenshot-dialog__img'))
118
this.dialog.addEventListener('click', this.handleBackdropClick)
129

1310
this.dialog.querySelector('[data-close-dialog]')
1411
?.addEventListener('click', () => this.dialog?.close())
1512

16-
this.dialog.querySelector('[data-prev]')
17-
?.addEventListener('click', () => this.show(this.current - 1))
18-
19-
this.dialog.querySelector('[data-next]')
20-
?.addEventListener('click', () => this.show(this.current + 1))
21-
2213
// Find the button that opens this dialog (outside this element, in the card)
2314
const dialogId = this.dialog.id
2415
if (dialogId) {
2516
const opener = document.querySelector(`[data-open-dialog="${dialogId}"]`)
26-
opener?.addEventListener('click', () => {
27-
this.show(0)
28-
this.dialog?.showModal()
29-
})
17+
opener?.addEventListener('click', () => this.dialog?.showModal())
3018
}
3119
}
3220

3321
disconnectedCallback() {
3422
this.dialog?.removeEventListener('click', this.handleBackdropClick)
3523
}
3624

37-
private show(index: number) {
38-
if (index < 0 || index >= this.images.length) return
39-
this.current = index
40-
41-
for (const img of this.images) {
42-
img.hidden = true
43-
}
44-
this.images[index].hidden = false
45-
46-
const counter = this.dialog?.querySelector('.screenshot-dialog__counter')
47-
if (counter && this.images.length > 1) {
48-
counter.textContent = `${index + 1} of ${this.images.length}`
49-
}
50-
51-
const prev = this.dialog?.querySelector('[data-prev]') as HTMLButtonElement | null
52-
const next = this.dialog?.querySelector('[data-next]') as HTMLButtonElement | null
53-
if (prev) prev.disabled = index === 0
54-
if (next) next.disabled = index === this.images.length - 1
55-
}
56-
5725
handleBackdropClick = (e: Event) => {
5826
if (e.target === this.dialog) {
5927
this.dialog.close()

tests/views/components.test.tsx

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -186,19 +186,13 @@ describe('LabCard', () => {
186186
expect(html).not.toContain('href="/assets/images/')
187187
})
188188

189-
test('screenshot link renders a dialog with images and nav buttons', async () => {
189+
test('screenshot link renders a dialog with images', async () => {
190190
const html = await renderToHtml(<LabCard lab={withScreenshots} />)
191191
expect(html).toContain('<dialog')
192192
expect(html).toContain('src="/assets/images/messaging-dashboard.png"')
193193
expect(html).toContain('alt="Dashboard"')
194194
expect(html).toContain('src="/assets/images/messaging-get-started.png"')
195195
expect(html).toContain('alt="Get started"')
196-
// First image visible, second hidden
197-
expect(html).toContain('data-index="0"')
198-
expect(html).toContain('data-index="1"')
199-
// Prev/next nav buttons
200-
expect(html).toContain('data-prev')
201-
expect(html).toContain('data-next')
202196
})
203197

204198
test('screenshot image src respects basePath', async () => {

0 commit comments

Comments
 (0)