Skip to content

Commit 3caa8b1

Browse files
refactor: address code review feedback
- Remove unnecessary Promise.all from image mapping - Use stable keys for React list items - Implement event delegation to prevent memory leaks - Add accessibility attributes (role, aria-modal, aria-label) - Add keyboard navigation support (Escape, Arrow keys)
1 parent f2b019d commit 3caa8b1

File tree

2 files changed

+72
-50
lines changed

2 files changed

+72
-50
lines changed

src/components/ui/ImageLightbox.tsx

Lines changed: 35 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useState } from 'react';
1+
import { useState, useEffect } from 'react';
22

33
interface ImageLightboxProps {
44
images: { src: string; alt?: string }[];
@@ -15,17 +15,28 @@ export default function ImageLightbox({
1515
}: ImageLightboxProps) {
1616
const [currentIndex, setCurrentIndex] = useState(initialIndex);
1717

18-
if (!isOpen) return null;
18+
useEffect(() => {
19+
const handleKeyDown = (e: KeyboardEvent) => {
20+
if (e.key === 'Escape') {
21+
onClose();
22+
} else if (e.key === 'ArrowRight') {
23+
goToNext();
24+
} else if (e.key === 'ArrowLeft') {
25+
goToPrev();
26+
}
27+
};
1928

20-
const goToNext = (e: React.MouseEvent) => {
21-
e.stopPropagation();
22-
setCurrentIndex((currentIndex + 1) % images.length);
23-
};
29+
if (isOpen) {
30+
document.addEventListener('keydown', handleKeyDown);
31+
return () => document.removeEventListener('keydown', handleKeyDown);
32+
}
33+
}, [isOpen, currentIndex]);
2434

25-
const goToPrev = (e: React.MouseEvent) => {
26-
e.stopPropagation();
35+
if (!isOpen) return null;
36+
37+
const goToNext = () => setCurrentIndex((currentIndex + 1) % images.length);
38+
const goToPrev = () =>
2739
setCurrentIndex((currentIndex - 1 + images.length) % images.length);
28-
};
2940

3041
const handleClose = (e: React.MouseEvent) => {
3142
e.stopPropagation();
@@ -36,11 +47,14 @@ export default function ImageLightbox({
3647
<div
3748
className="fixed inset-0 z-50 bg-black/90 flex items-center justify-center"
3849
onClick={onClose}
50+
role="dialog"
51+
aria-modal="true"
52+
aria-label="Image gallery"
3953
>
4054
<button
4155
onClick={handleClose}
4256
className="absolute top-4 right-4 text-white text-2xl hover:text-gray-300 z-10"
43-
aria-label="Close"
57+
aria-label="Close gallery"
4458
>
4559
×
4660
</button>
@@ -56,23 +70,29 @@ export default function ImageLightbox({
5670
{images.length > 1 && (
5771
<>
5872
<button
59-
onClick={goToPrev}
73+
onClick={(e) => {
74+
e.stopPropagation();
75+
goToPrev();
76+
}}
6077
className="absolute left-4 text-white text-4xl hover:text-gray-300"
61-
aria-label="Previous"
78+
aria-label="Previous image"
6279
>
6380
6481
</button>
6582
<button
66-
onClick={goToNext}
83+
onClick={(e) => {
84+
e.stopPropagation();
85+
goToNext();
86+
}}
6787
className="absolute right-4 text-white text-4xl hover:text-gray-300"
68-
aria-label="Next"
88+
aria-label="Next image"
6989
>
7090
7191
</button>
7292
<div className="absolute bottom-4 flex gap-2">
7393
{images.map((_, idx) => (
7494
<button
75-
key={idx}
95+
key={images[idx].src}
7696
onClick={(e) => {
7797
e.stopPropagation();
7898
setCurrentIndex(idx);

src/components/ui/WorkExperience.astro

Lines changed: 37 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,10 @@ const { entry } = Astro.props;
99
const { Content } = await render(entry);
1010
1111
const images = entry.data.images
12-
? await Promise.all(
13-
entry.data.images.map(async (img) => ({
14-
src: img.src,
15-
alt: entry.data.title,
16-
}))
17-
)
12+
? entry.data.images.map((img) => ({
13+
src: img.src,
14+
alt: entry.data.title,
15+
}))
1816
: [];
1917
---
2018

@@ -58,34 +56,38 @@ const images = entry.data.images
5856
import { createRoot } from 'react-dom/client';
5957
import { createElement } from 'react';
6058

61-
const thumbs = document.querySelectorAll('.work-image-thumb');
59+
const container = document.querySelector('.flex.gap-2.flex-wrap.mt-2');
60+
if (!container) return;
6261

63-
thumbs.forEach((thumb) => {
64-
thumb.addEventListener('click', () => {
65-
const images = JSON.parse(thumb.getAttribute('data-images') || '[]');
66-
const index = parseInt(thumb.getAttribute('data-index') || '0');
67-
68-
const container = document.createElement('div');
69-
document.body.appendChild(container);
70-
const root = createRoot(container);
71-
72-
const closeHandler = () => {
73-
root.unmount();
74-
try {
75-
document.body.removeChild(container);
76-
} catch (e) {
77-
// Container may have already been removed; ignore error
78-
}
79-
};
80-
81-
root.render(
82-
createElement(ImageLightbox, {
83-
images,
84-
isOpen: true,
85-
initialIndex: index,
86-
onClose: closeHandler,
87-
})
88-
);
89-
});
90-
});
62+
const clickHandler = (event: Event) => {
63+
const thumb = (event.target as HTMLElement).closest('.work-image-thumb');
64+
if (!thumb || !container.contains(thumb)) return;
65+
66+
const images = JSON.parse(thumb.getAttribute('data-images') || '[]');
67+
const index = parseInt(thumb.getAttribute('data-index') || '0');
68+
69+
const lightboxContainer = document.createElement('div');
70+
document.body.appendChild(lightboxContainer);
71+
const root = createRoot(lightboxContainer);
72+
73+
const closeHandler = () => {
74+
root.unmount();
75+
try {
76+
document.body.removeChild(lightboxContainer);
77+
} catch (e) {
78+
// Container may have already been removed; ignore error
79+
}
80+
};
81+
82+
root.render(
83+
createElement(ImageLightbox, {
84+
images,
85+
isOpen: true,
86+
initialIndex: index,
87+
onClose: closeHandler,
88+
})
89+
);
90+
};
91+
92+
container.addEventListener('click', clickHandler);
9193
</script>

0 commit comments

Comments
 (0)