rt/wrolpi
1
0
Fork 0
mirror of https://github.com/lrnselfreliance/wrolpi.git synced 2026-10-26 19:01:39 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions

review: name the icon-only controls, and make two guards mean what they say

Browse source 
Acting on the review of #626.  No bugs; one suggestion and three nits.

The map's icon-only controls had no accessible name.  IconButton requires a
label by construction and AddToPlaylistButton supplies one, but the plain Button
and APIButton used for edit, delete and the catalog preview reached assistive tech
as unnamed buttons -- and my own Cypress fixtures carefully passed `aria-label`
on the same pattern while production did not, which is its own kind of tell.  All
four are named now, and named with the row's subject rather than the verb alone:
"Delete Water tank", not "Delete", because a table of pins otherwise offers a
screen reader a column of identical buttons.

A source guard holds that: it scans for controls that carry an icon and no label
text, and reports any without an `aria-label`.  Paired with a premise that it
found some, since a scanner matching nothing reads exactly like a clean file.

The Group guard asserted only that `<Group` appeared somewhere in the cell, which
would pass on an empty Group left beside the bare siblings it was meant to wrap.
It now checks that every control in the cell is inside the Group -- and the first
version of THAT was wrong too: it sliced from `<Group` to the end of the cell
rather than to `</Group>`, so controls after the closing tag still counted as
inside, and the empty-Group plant stayed green.  Slicing between the tags makes
it fail.

Coverage the review asked for: the Semantic size names the call sites actually
use (`small`, `big`, `huge`) rather than only the Mantine ones, since the
translation step was untested on the exact spellings that were broken; icon-only
buttons stay square while loading, when the glyph is replaced by a spinner sized
from the button's height; and `iconAfter`-only squares as well as `icon`-only.

On the size passthrough, which was flagged as the highest-risk change outside the
map: the buttons that grow are Settings' Shutdown and Reboot (`huge`, to 60px),
Settings' Upgrade and Save and Channels' Save (`big`, to 50px), and VideoPlayer's
Refresh (`large`, to 50px).  All six read as deliberate emphasis -- the two
biggest are the two destructive ones -- so they are left honoured rather than
re-broken at the call site.  `size='small'` resolves to `sm`, which is what the
accidental default already was, so the detail pages do not move.

1134 jest / 64 suites, 246 in ui-layout.cy.js, clean npm run build.
This commit is contained in:
Roland 2026-08-02 17:57:27 -06:00
parent b0a7c9da1e
commit f997f730e1
No known key found for this signature in database
GPG key ID: 1BA620CE6C9A4680
3 changed files with 146 additions and 3 deletions
Download patch file Download diff file Expand all files Collapse all files

6
app/src/components/Map.js
View file

@ -235,6 +235,7 @@ function MapCatalogRow({item, subscribedRegions, fetchData}) {
{bbox && <Button
size='xs'
icon='eye'
aria-label={`Preview ${name}`}
onClick={() => setPreviewOpen(true)}
/>}
<Button
@ -535,7 +536,8 @@ function MapPins() {
<Table.Cell>
{/* See PinEditRow: a Group so the three controls line up and are spaced. */}
<Group gap='xs' wrap='nowrap'>
<Button size='xs' icon='edit' onClick={() => setEditingId(pin.id)}/>
<Button size='xs' icon='edit' aria-label={`Edit ${pin.label}`}
onClick={() => setEditingId(pin.id)}/>
<AddToPlaylistButton
size='xs'
icon='list'
@ -546,6 +548,7 @@ function MapPins() {
size='xs'
role='danger'
icon='trash'
aria-label={`Delete ${pin.label}`}
confirmContent='Delete this pin?'
confirmButton='Delete'
onClick={() => handleDelete(pin.id)}
@ -581,6 +584,7 @@ function MapPins() {
size='xs'
role='danger'
icon='trash'
aria-label={`Delete ${pin.label}`}
confirmContent='Delete this pin?'
confirmButton='Delete'
onClick={() => handleDelete(pin.id)}

84
app/src/components/Map.test.js
View file

@ -71,8 +71,88 @@ describe('the map pin action cells', () => {
* message argument -- that is chai, and passing one throws rather than annotating.
* A rename of the marker would otherwise turn this into a test of nothing.
*/
expect({marker, found: cell !== undefined, grouped: !!cell?.includes('<Group')})
.toEqual({marker, found: true, grouped: true});
/*
* The Group must CONTAIN the cell's controls, not merely appear in it. Asserting
* that `<Group` is present anywhere would pass on an empty one left beside the
* bare siblings it was supposed to wrap.
*/
const between = (text, open, close) => {
const from = text.indexOf(open);
if (from === -1) return '';
const to = text.indexOf(close, from);
// An unclosed Group would otherwise swallow the rest of the cell, which is how
// the first version of this let an EMPTY Group pass with the controls left
// outside it.
return to === -1 ? '' : text.slice(from, to);
};
const inGroup = cell === undefined ? '' : between(cell, '<Group', '</Group>');
const controls = (text) =>
(text.match(/<(?:API)?Button\b|<IconButton\b|<AddToPlaylistButton\b/g) || []).length;
expect({
marker,
found: cell !== undefined,
grouped: !!cell?.includes('<Group'),
// Every control the cell has is inside the Group, none left outside it.
controlsInsideGroup: cell === undefined ? 0 : controls(inGroup),
controlsInCell: cell === undefined ? -1 : controls(cell),
}).toEqual({
marker, found: true, grouped: true,
controlsInsideGroup: cell === undefined ? 0 : controls(cell),
controlsInCell: cell === undefined ? -1 : controls(cell),
});
});
});
});
describe('the map\'s icon-only controls have accessible names', () => {
/*
* An icon-only Button carries no text, so without `aria-label` it reaches assistive tech
* as an unnamed button. `IconButton` requires a label by construction and
* AddToPlaylistButton supplies one; the plain Button and APIButton used for edit, delete
* and the catalog preview did not, and this change rewrote all three rows.
*
* In the source, for the same reason as the Group guard: these rows are built inside the
* module and fetched from the API.
*/
const openingTag = (text, at) => {
let depth = 0;
for (let i = at; i < text.length; i++) {
const c = text[i];
if (c === '{' || c === '(') depth += 1;
else if (c === '}' || c === ')') depth -= 1;
else if (c === '>' && depth === 0) return {tag: text.slice(at, i + 1), end: i};
}
return {tag: '', end: at};
};
/** Every `<Button>`/`<APIButton>` in the file whose only content is an icon. */
const iconOnlyControls = () => {
const found = [];
for (const match of source.matchAll(/<(?:API)?Button\b/g)) {
const {tag, end} = openingTag(source, match.index);
if (!/\bicon=/.test(tag)) continue;
// Self-closing, or immediately followed by another tag rather than by a label.
const following = source.slice(end + 1, end + 40).trim();
if (!(tag.endsWith('/>') || following.startsWith('<') || following.startsWith('}'))) {
continue;
}
found.push({line: source.slice(0, match.index).split('\n').length, tag});
}
return found;
};
it('finds the icon-only controls at all', () => {
// The premise. A scanner that matched nothing would report no offenders and read
// exactly like a file that was already correct.
expect(iconOnlyControls().length).toBeGreaterThan(2);
});
it('names every one of them', () => {
const unnamed = iconOnlyControls()
.filter(({tag}) => !/aria-label/.test(tag))
.map(({line, tag}) => `line ${line}: ${tag.split('\n')[0].trim()}`);
expect(unnamed).toEqual([]);
});
});

59
app/src/components/ui/ui-layout.cy.js
View file

@ -3457,6 +3457,65 @@ describe('APIButton honours the size its call site asks for', () => {
});
});
it('honours the Semantic size names the call sites actually use', () => {
/*
* The seventeen call sites that were being ignored do not say `xs` and `lg` -- they say
* `small`, `big`, `large` and `huge`, which `resolveSize` translates. Testing only the
* Mantine names would leave the translation step untested on the exact spellings that
* were broken. `small` maps to `sm`, which is what the accidental default already was,
* which is why most detail pages do not move.
*/
cy.mountUI(<div>
<APIButton size='small' icon='trash' onClick={() => {}} data-testid='small'/>
<APIButton size='big' icon='trash' onClick={() => {}} data-testid='big'/>
<APIButton size='huge' icon='trash' onClick={() => {}} data-testid='huge'/>
</div>);
cy.get('[data-testid="huge"]').should(($huge) => {
const height = (id) =>
Cypress.$(`[data-testid="${id}"]`)[0].getBoundingClientRect().height;
expect(height('small'), 'small is the old default').to.be.closeTo(36, 1);
expect(height('big'), 'big is taller than small').to.be.greaterThan(height('small'));
expect($huge[0].getBoundingClientRect().height, 'huge is taller than big')
.to.be.greaterThan(height('big'));
});
});
it('stays square while it is loading', () => {
/*
* A loading button swaps its glyph for a spinner sized from the button's height, and
* the square rule fixes the width to that height -- so a spinner that did not fit
* would either be clipped or stretch the box. Both Button and APIButton, since
* APIButton is what actually spends time loading.
*/
cy.mountUI(<div>
<Button size='xs' icon='trash' aria-label='Delete' loading data-testid='busy'/>
<APIButton size='xs' icon='trash' loading onClick={() => {}} data-testid='busy-api'/>
</div>);
cy.get('[data-testid="busy-api"]').should(() => {
['busy', 'busy-api'].forEach((id) => {
const box = Cypress.$(`[data-testid="${id}"]`)[0].getBoundingClientRect();
expect(box.height, `${id} has a height`).to.be.greaterThan(20);
expect(box.width, `${id} is still square`).to.be.closeTo(box.height, 1);
});
});
});
it('squares an iconAfter-only button too', () => {
// The square rule keys off the same `data-icon-only` marker as the centring, and
// `iconAfter` alone is one of the cases that sets it.
cy.mountUI(<Button size='xs' iconAfter='upload' aria-label='Send' data-testid='after'/>);
cy.get('[data-testid="after"]').should(($button) => {
const box = $button[0].getBoundingClientRect();
expect(box.width, 'square').to.be.closeTo(box.height, 1);
});
});
it('is a real constraint: an APIButton with no size keeps the default it had', () => {
/*
* `useAPIButton` declared `size = 'medium'` as a parameter default while never using