Skip to content

Commit 86f8bbb

Browse files
Copilotintel352
andauthored
fix: address DataTable, Modal, and ErrorBoundary review feedback (#9)
* Initial plan * fix: address PR review comments for DataTable, Modal, and ErrorBoundary Co-authored-by: intel352 <77607+intel352@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: intel352 <77607+intel352@users.noreply.github.com>
1 parent 391098e commit 86f8bbb

File tree

6 files changed

+44
-34
lines changed

6 files changed

+44
-34
lines changed

package-lock.json

Lines changed: 19 additions & 10 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

src/components/DataTable/DataTable.test.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ describe('DataTable', () => {
4747
caption="Workflow list"
4848
/>,
4949
);
50-
expect(screen.getByRole('grid', { name: 'Workflow list' })).toBeInTheDocument();
50+
expect(screen.getByRole('table', { name: 'Workflow list' })).toBeInTheDocument();
5151
});
5252

5353
it('sorts by column when clicking sortable header', async () => {

src/components/DataTable/DataTable.tsx

Lines changed: 14 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
1-
import { useState, useCallback, type CSSProperties } from 'react';
1+
import { useState, useCallback, useEffect, type CSSProperties, type ReactNode } from 'react';
22
import { colors, baseStyles } from '../../theme';
33

44
export interface DataTableColumn<T> {
55
/** Unique key matching a property in the row data. */
6-
key: string;
6+
key: keyof T & string;
77
/** Display header text. */
88
header: string;
99
/** Whether this column is sortable. Default: false */
1010
sortable?: boolean;
1111
/** Custom cell renderer. */
12-
render?: (value: unknown, row: T) => React.ReactNode;
12+
render?: (value: T[keyof T & string], row: T) => ReactNode;
1313
}
1414

1515
export interface DataTableProps<T> {
@@ -37,12 +37,20 @@ export default function DataTable<T extends Record<string, unknown>>({
3737
caption,
3838
style,
3939
}: DataTableProps<T>) {
40-
const [sortKey, setSortKey] = useState<string | null>(null);
40+
const [sortKey, setSortKey] = useState<(keyof T & string) | null>(null);
4141
const [sortDir, setSortDir] = useState<SortDirection>('asc');
4242
const [page, setPage] = useState(0);
4343

44+
// Clamp page when data length or pageSize changes to avoid empty views
45+
useEffect(() => {
46+
if (pageSize > 0) {
47+
const total = Math.max(1, Math.ceil(data.length / pageSize));
48+
setPage((p) => Math.min(p, total - 1));
49+
}
50+
}, [data.length, pageSize]);
51+
4452
const handleSort = useCallback(
45-
(key: string) => {
53+
(key: keyof T & string) => {
4654
if (sortKey === key) {
4755
setSortDir((d) => (d === 'asc' ? 'desc' : 'asc'));
4856
} else {
@@ -73,7 +81,7 @@ export default function DataTable<T extends Record<string, unknown>>({
7381

7482
return (
7583
<div style={style}>
76-
<table style={baseStyles.table} role="grid" aria-label={caption}>
84+
<table style={baseStyles.table} aria-label={caption}>
7785
{caption && <caption style={{ ...visuallyHidden }}>{caption}</caption>}
7886
<thead>
7987
<tr>

src/components/ErrorBoundary/ErrorBoundary.test.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { describe, it, expect, vi } from 'vitest';
1+
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
22
import { render, screen } from '@testing-library/react';
33
import userEvent from '@testing-library/user-event';
44
import ErrorBoundary from './ErrorBoundary';

src/components/Modal/Modal.test.tsx

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,8 +44,10 @@ describe('Modal', () => {
4444
</Modal>,
4545
);
4646
const dialog = screen.getByRole('dialog');
47-
expect(dialog).toHaveAttribute('aria-labelledby', 'modal-title');
48-
expect(screen.getByText('My Dialog')).toHaveAttribute('id', 'modal-title');
47+
const labelledById = dialog.getAttribute('aria-labelledby');
48+
expect(labelledById).toBeTruthy();
49+
const titleEl = screen.getByText('My Dialog');
50+
expect(titleEl).toHaveAttribute('id', labelledById);
4951
});
5052

5153
it('calls onClose when close button is clicked', async () => {

src/components/Modal/Modal.tsx

Lines changed: 5 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { useEffect, useRef, useCallback, type CSSProperties, type ReactNode } from 'react';
1+
import { useEffect, useRef, useCallback, useId, type CSSProperties, type ReactNode, type MouseEvent } from 'react';
22
import { colors, baseStyles } from '../../theme';
33

44
export type ModalVariant = 'info' | 'confirmation' | 'error';
@@ -43,6 +43,7 @@ export default function Modal({
4343
}: ModalProps) {
4444
const dialogRef = useRef<HTMLDialogElement>(null);
4545
const previousFocusRef = useRef<HTMLElement | null>(null);
46+
const titleId = useId();
4647

4748
useEffect(() => {
4849
const dialog = dialogRef.current;
@@ -57,15 +58,6 @@ export default function Modal({
5758
}
5859
}, [open]);
5960

60-
const handleKeyDown = useCallback(
61-
(e: React.KeyboardEvent) => {
62-
if (e.key === 'Escape') {
63-
onClose();
64-
}
65-
},
66-
[onClose],
67-
);
68-
6961
// Handle native dialog cancel event (ESC key)
7062
useEffect(() => {
7163
const dialog = dialogRef.current;
@@ -79,7 +71,7 @@ export default function Modal({
7971
}, [onClose]);
8072

8173
const handleBackdropClick = useCallback(
82-
(e: React.MouseEvent) => {
74+
(e: MouseEvent<HTMLDialogElement>) => {
8375
if (e.target === dialogRef.current) {
8476
onClose();
8577
}
@@ -94,9 +86,8 @@ export default function Modal({
9486
return (
9587
<dialog
9688
ref={dialogRef}
97-
onKeyDown={handleKeyDown}
9889
onClick={handleBackdropClick}
99-
aria-labelledby="modal-title"
90+
aria-labelledby={titleId}
10091
style={{
10192
backgroundColor: colors.surface0,
10293
color: colors.text,
@@ -117,7 +108,7 @@ export default function Modal({
117108
}}
118109
>
119110
<h2
120-
id="modal-title"
111+
id={titleId}
121112
style={{
122113
margin: 0,
123114
fontSize: '18px',

0 commit comments

Comments
 (0)