IssueReportPage: unmount guard on the repos fetch, aria-sort on sortable headers
Two non-blocking notes from review: - the /api/repos effect now uses the same cancelled guard the repo-filter effect already has, so an unmount mid-flight doesn't call setRepos/setError on a gone component. - Table's TableColumn gains an optional ariaSort field, consumed as the <th>'s aria-sort attribute; the issue-report page's sortable columns now report ascending/descending/none so a screen reader can announce which column and direction is active, not just the sighted ▲/▼ glyph.
This commit is contained in:
parent
f4b61db753
commit
6e6bf62437
2 changed files with 43 additions and 5 deletions
|
|
@ -109,16 +109,26 @@ export function IssueReportPage() {
|
||||||
|
|
||||||
// Repo dropdown source, fetched once — this page has no refresh
|
// Repo dropdown source, fetched once — this page has no refresh
|
||||||
// cadence, and a repo gaining/losing its first/last open issue between
|
// cadence, and a repo gaining/losing its first/last open issue between
|
||||||
// visits is rare enough that a manual reload covers it.
|
// visits is rare enough that a manual reload covers it. Same
|
||||||
|
// `cancelled` guard as the repo-filter effect below, so an unmount
|
||||||
|
// mid-flight (navigating away before this resolves) doesn't call
|
||||||
|
// `setRepos`/`setError` on a component that's already gone.
|
||||||
useEffect(() => {
|
useEffect(() => {
|
||||||
|
let cancelled = false;
|
||||||
(async () => {
|
(async () => {
|
||||||
const r = await fetch('/api/repos');
|
const r = await fetch('/api/repos');
|
||||||
if (!r.ok) {
|
if (!r.ok) {
|
||||||
setError(await readApiError(r));
|
if (!cancelled) setError(await readApiError(r));
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
setRepos((await r.json()) as string[]);
|
const data = (await r.json()) as string[];
|
||||||
})().catch((e: unknown) => setError({ detail: String(e) }));
|
if (!cancelled) setRepos(data);
|
||||||
|
})().catch((e: unknown) => {
|
||||||
|
if (!cancelled) setError({ detail: String(e) });
|
||||||
|
});
|
||||||
|
return () => {
|
||||||
|
cancelled = true;
|
||||||
|
};
|
||||||
}, []);
|
}, []);
|
||||||
|
|
||||||
// The one fetch this page re-runs on state change — everything else
|
// The one fetch this page re-runs on state change — everything else
|
||||||
|
|
@ -188,6 +198,15 @@ export function IssueReportPage() {
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// `aria-sort` for a sortable column's `<th>` — 'none' while sortable
|
||||||
|
// but not the active column, the real direction while it is. Screen
|
||||||
|
// readers announce this; the ▲/▼ glyph in `SortHeader` is `aria-hidden`
|
||||||
|
// and carries no information without it.
|
||||||
|
function ariaSortFor(key: SortKey): 'ascending' | 'descending' | 'none' {
|
||||||
|
if (sortKey !== key) return 'none';
|
||||||
|
return sortDir === 'asc' ? 'ascending' : 'descending';
|
||||||
|
}
|
||||||
|
|
||||||
const columns: TableColumn<IssueReportRow>[] = [
|
const columns: TableColumn<IssueReportRow>[] = [
|
||||||
// Shown even with one repo selected (every row shares the value) —
|
// Shown even with one repo selected (every row shares the value) —
|
||||||
// a fixed column set means the table's shape doesn't shift under
|
// a fixed column set means the table's shape doesn't shift under
|
||||||
|
|
@ -195,11 +214,13 @@ export function IssueReportPage() {
|
||||||
{
|
{
|
||||||
key: 'repo',
|
key: 'repo',
|
||||||
header: <SortHeader label="repo" sortKey="repo" activeKey={sortKey} dir={sortDir} onSort={onSort} />,
|
header: <SortHeader label="repo" sortKey="repo" activeKey={sortKey} dir={sortDir} onSort={onSort} />,
|
||||||
|
ariaSort: ariaSortFor('repo'),
|
||||||
render: (r) => r.repo,
|
render: (r) => r.repo,
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
key: 'number',
|
key: 'number',
|
||||||
header: <SortHeader label="issue" sortKey="number" activeKey={sortKey} dir={sortDir} onSort={onSort} />,
|
header: <SortHeader label="issue" sortKey="number" activeKey={sortKey} dir={sortDir} onSort={onSort} />,
|
||||||
|
ariaSort: ariaSortFor('number'),
|
||||||
render: (r) =>
|
render: (r) =>
|
||||||
r.html_url ? (
|
r.html_url ? (
|
||||||
<a href={r.html_url} target="_blank" rel="noreferrer">
|
<a href={r.html_url} target="_blank" rel="noreferrer">
|
||||||
|
|
@ -212,6 +233,7 @@ export function IssueReportPage() {
|
||||||
{
|
{
|
||||||
key: 'title',
|
key: 'title',
|
||||||
header: <SortHeader label="title" sortKey="title" activeKey={sortKey} dir={sortDir} onSort={onSort} />,
|
header: <SortHeader label="title" sortKey="title" activeKey={sortKey} dir={sortDir} onSort={onSort} />,
|
||||||
|
ariaSort: ariaSortFor('title'),
|
||||||
render: (r) => r.title,
|
render: (r) => r.title,
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
|
|
@ -224,11 +246,13 @@ export function IssueReportPage() {
|
||||||
header: (
|
header: (
|
||||||
<SortHeader label="assignees" sortKey="assignees" activeKey={sortKey} dir={sortDir} onSort={onSort} />
|
<SortHeader label="assignees" sortKey="assignees" activeKey={sortKey} dir={sortDir} onSort={onSort} />
|
||||||
),
|
),
|
||||||
|
ariaSort: ariaSortFor('assignees'),
|
||||||
render: (r) => (r.assignees.length ? r.assignees.join(', ') : '—'),
|
render: (r) => (r.assignees.length ? r.assignees.join(', ') : '—'),
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
key: 'blocked',
|
key: 'blocked',
|
||||||
header: <SortHeader label="blocked" sortKey="blocked" activeKey={sortKey} dir={sortDir} onSort={onSort} />,
|
header: <SortHeader label="blocked" sortKey="blocked" activeKey={sortKey} dir={sortDir} onSort={onSort} />,
|
||||||
|
ariaSort: ariaSortFor('blocked'),
|
||||||
render: (r) => (r.blocked ? <Badge tone="warning" value="blocked" /> : '—'),
|
render: (r) => (r.blocked ? <Badge tone="warning" value="blocked" /> : '—'),
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
|
|
@ -242,6 +266,7 @@ export function IssueReportPage() {
|
||||||
onSort={onSort}
|
onSort={onSort}
|
||||||
/>
|
/>
|
||||||
),
|
),
|
||||||
|
ariaSort: ariaSortFor('depended_on_by_count'),
|
||||||
render: (r) => r.depended_on_by_count,
|
render: (r) => r.depended_on_by_count,
|
||||||
},
|
},
|
||||||
];
|
];
|
||||||
|
|
|
||||||
|
|
@ -20,6 +20,17 @@ export interface TableColumn<T> {
|
||||||
// can pass real markup instead of forking a second table primitive.
|
// can pass real markup instead of forking a second table primitive.
|
||||||
header: ComponentChildren;
|
header: ComponentChildren;
|
||||||
render: (row: T) => ComponentChildren;
|
render: (row: T) => ComponentChildren;
|
||||||
|
/**
|
||||||
|
* ARIA sort state for this column's `<th>` — `'ascending'` /
|
||||||
|
* `'descending'` while this is the active sort column, `'none'` while
|
||||||
|
* sortable but not active, omitted entirely for a non-sortable column
|
||||||
|
* (no `aria-sort` attribute at all, the correct value for a column
|
||||||
|
* that can never be the active sort). A screen reader announces which
|
||||||
|
* column/direction is active from this attribute; the ▲/▼ glyph a
|
||||||
|
* sortable header renders is `aria-hidden` and carries no information
|
||||||
|
* on its own.
|
||||||
|
*/
|
||||||
|
ariaSort?: 'ascending' | 'descending' | 'none';
|
||||||
}
|
}
|
||||||
|
|
||||||
export function Table<T>({
|
export function Table<T>({
|
||||||
|
|
@ -44,7 +55,9 @@ export function Table<T>({
|
||||||
<thead>
|
<thead>
|
||||||
<tr>
|
<tr>
|
||||||
{columns.map((c) => (
|
{columns.map((c) => (
|
||||||
<th key={c.key}>{c.header}</th>
|
<th key={c.key} aria-sort={c.ariaSort}>
|
||||||
|
{c.header}
|
||||||
|
</th>
|
||||||
))}
|
))}
|
||||||
</tr>
|
</tr>
|
||||||
</thead>
|
</thead>
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue