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: one inset, and two tests that could not fail

Browse source 
Acting on the review of #625.  No bugs; four suggestions and three nits, of
which two were already handled and one needed no code.

The weld used three separate 8px literals -- the control's padding, its flex gap,
and the clear button's negative margins -- which had to stay in lockstep or the
button would drift past the border or back inside it.  They are one
`--searchbox-inset` now.

Cancelling the gap on the clear button's LEFT was a real side effect: when a
field is both `loading` and `clearable`, which SearchResultsInput allows, the DOM
order is input, spinner, clear, and the spinner ended up welded to the divider.
Only the right-hand padding is cancelled now; the 8px before the button stays,
and a case covers the pair.

`min-height` was still coming from `--ai-size`, so the rule only half-owned the
height it claimed to set -- true today by 0.15px, the field being 36.15 against a
36 minimum.  Unset now.

Two nits needed no code and are recorded as findings rather than changes.  The
clear button's corners are already square: the theme zeroes every
`--mantine-radius-*`, so there are no wedges to show against the field, and a
case now depends on that explicitly.  And a label-less button with BOTH `icon`
and `iconAfter` would stay mis-centred, but no call site writes one, so the limit
is documented instead of designed around.

Blank children now count as no label.  `toArray` drops null and booleans but
keeps `''` and whitespace, which would have left the glyph off-centre; no call
site does it today and the predicate should not depend on that.

Two corrections to my own work, both caught by planting:

The fragment case.  I wrote that `toArray` flattens fragments away to nothing and
tested `<></>` as unlabelled.  It does not -- it returns the fragment as one
child, so `<></>` is indistinguishable from real content.  The test now records
what actually happens, with `<>Delete</>` as the shape that occurs in practice.

The min-height assertion compared `--ai-size` -- which reads back as
`calc(2.25rem * var(--mantine-scale))` -- against a computed `36px`.  Two strings
that can never match, so it passed with the rule removed.  It compares resolved
lengths now, paired with an ordinary IconButton that DOES carry the minimum, so
its absence is attributable to this rule rather than to a Mantine default.  An
earlier attempt fabricated a shorter field by overriding the input's font size;
that was deleted, because a test that invents a layout the app never renders is
not evidence about this app.

1128 jest / 63 suites, 231 in ui-layout.cy.js, clean npm run build.  Confirmed
live: inset 8px, clear button 1px from the field edge, 8px from the input, at the
field's full inner height with no minimum of its own.
This commit is contained in:
Roland 2026-08-02 17:28:46 -06:00 • committed by lrnselfreliance
parent feaca59ce5
commit 83dc2ac82f
3 changed files with 153 additions and 10 deletions
Download patch file Download diff file Expand all files Collapse all files

15
app/src/components/ui/Button.tsx
View file

@ -83,7 +83,20 @@ export const Button = forwardRef<HTMLButtonElement, ButtonProps>((
* too narrow for words. Those buttons are icon-only at exactly the widths where the
* offset is most visible.
*/
const labelled = React.Children.toArray(children).length > 0;
/*
* Blank text is not a label. `toArray` drops null, undefined and booleans, but keeps an
* empty or whitespace-only string, which would leave the icon in `leftSection` with its
* margin and the glyph off-centre again. No call site does that today; the predicate
* should not depend on that staying true.
*
* Two limits, both deliberate. "Icon-only" means exactly one of `icon`/`iconAfter`: a
* label-less button with both would keep both section margins and stay mis-centred, and
* there are no such call sites to design for. And any ELEMENT child counts as a label,
* including an empty fragment -- `toArray` does not look inside one, so `<></>` is
* indistinguishable from real content without recursing into fragment props.
*/
const labelled = React.Children.toArray(children)
.some(child => typeof child !== 'string' || child.trim().length > 0);
const soleIcon = !labelled && (icon || iconAfter) && !(icon && iconAfter)
? renderIcon(icon || iconAfter)
: undefined;

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

@ -3105,6 +3105,56 @@ describe('a button with an icon and no label centres the icon', () => {
});
});
it('treats blank and empty children as no label', () => {
/*
* `Children.toArray` drops null, undefined and booleans but KEEPS an empty or
* whitespace-only string, so a length check alone would call these labelled and leave
* the glyph off-centre.
*/
cy.mountUI(<div>
<Button icon='upload' aria-label='Empty' data-testid='empty'>{''}</Button>
<Button icon='upload' aria-label='Blank' data-testid='blank'>{' '}</Button>
</div>);
cy.get('[data-testid="blank"]').should(() => {
['empty', 'blank'].forEach((id) => {
const button = Cypress.$(`[data-testid="${id}"]`)[0];
const box = button.getBoundingClientRect();
const glyph = button.querySelector('svg').getBoundingClientRect();
expect((glyph.left + glyph.width / 2) - (box.left + box.width / 2),
`${id} glyph centre`).to.be.closeTo(0, 1);
});
});
});
it('treats any element child as a label, including an empty fragment', () => {
/*
* Recording what actually happens rather than what would be tidy. `toArray` does not
* look inside a fragment -- it returns the fragment itself as one child -- so `<></>`
* is indistinguishable from real content and the icon stays in `leftSection`.
*
* Detecting it would mean recursing into fragment props, and no call site writes an
* empty fragment as a button's children. `<>Delete</>` is the shape that does occur,
* and it lands on the same path for the right reason.
*/
cy.mountUI(<div>
<Button icon='trash' data-testid='fragment'><></></Button>
<Button icon='trash' data-testid='wrapped'><>Delete</></Button>
</div>);
cy.get('[data-testid="wrapped"]').should(() => {
['fragment', 'wrapped'].forEach((id) => {
const button = Cypress.$(`[data-testid="${id}"]`)[0];
const box = button.getBoundingClientRect();
const glyph = button.querySelector('svg').getBoundingClientRect();
expect((glyph.left + glyph.width / 2) - (box.left + box.width / 2),
`${id} keeps the icon leading`).to.be.lessThan(-3);
});
});
});
it('centres a trailing-only icon too', () => {
// `rightSection` has the mirror-image margin.
cy.mountUI(<Button iconAfter='upload' aria-label='Send' data-testid='after-only'/>);
@ -3225,6 +3275,73 @@ describe('the search field\'s clear button is attached to it', () => {
});
});
it('keeps the loading spinner off the clear button', () => {
/*
* `SearchResultsInput` can pass `loading` and `clearable` together, and the DOM order
* is input, spinner, clear. An earlier version of the weld cancelled the flex gap on
* the clear button's LEFT as well as the padding on its right, which welded the
* spinner to the divider -- the spinner is not part of this control.
*/
cy.mountUI(<div style={{width: 420}}>
<SearchBox value='wind turbine' clearable loading onChange={() => {}}/>
</div>);
cy.get('.wrolpi-searchbox-loading').should(($spinner) => {
const clear = Cypress.$('.wrolpi-searchbox-clear')[0].getBoundingClientRect();
expect(clear.left - $spinner[0].getBoundingClientRect().right,
'gap between the spinner and the divider').to.be.greaterThan(2);
});
});
it('owns the height in both directions, not just downwards', () => {
/*
* `align-self: stretch` cannot shrink a box below its own `min-height`, and Mantine
* sets that from `--ai-size` alongside `height`. Overriding `height` alone owns the
* sizing in one direction only -- it happens to look right because the field is
* 36.15px against a 36px minimum, a margin of 0.15px.
*
* The property rather than the geometry, deliberately. Making the field genuinely
* shorter than 36px means overriding the input's own font size and padding, which no
* part of the app does, and a test that fabricates a layout to prove a rule is not
* evidence about this app. This asserts what the rule is for: the minimum is no
* longer `--ai-size`. Removing `min-height: unset` fails it.
*/
cy.mountUI(<div style={{width: 420}}>
<SearchBox value='wind turbine' clearable onChange={() => {}}/>
{/* An ActionIcon outside the field, to show where the minimum comes from. */}
<IconButton icon='trash' label='Delete' data-testid='plain'/>
</div>);
cy.get('.wrolpi-searchbox-clear').should(($clear) => {
/*
* Resolved lengths, not the raw variable: `--ai-size` reads back as
* `calc(2.25rem * var(--mantine-scale))` while `min-height` computes to `36px`, so
* comparing the two strings can never match and asserts nothing. The first
* version of this did exactly that and stayed green with the rule removed.
*/
const minimum = (element) => parseFloat(getComputedStyle(element).minHeight) || 0;
// The inverse and the premise together: an ordinary IconButton DOES carry the
// minimum, so its absence here is this rule's doing and not Mantine's default.
expect(minimum(Cypress.$('[data-testid="plain"]')[0]),
'an ordinary icon button has a minimum height').to.be.greaterThan(0);
expect(minimum($clear[0]), 'the welded one does not').to.equal(0);
});
});
it('has square corners, so nothing shows through at the field edge', () => {
// A rounded ActionIcon stretched flush to a square field leaves panel-coloured wedges
// at the corners. Nothing sets this here: the theme zeroes every radius, and this
// records that the weld depends on it.
mountField();
cy.get('.wrolpi-searchbox-clear').should(($clear) => {
expect(getComputedStyle($clear[0]).borderRadius, 'no rounded corners')
.to.equal('0px');
});
});
it('is a real constraint: the field itself is still padded', () => {
/*
* The negative margins are aimed at one child. If the control's padding had simply

31
app/src/components/ui/ui.css
View file

@ -307,10 +307,17 @@ html[data-theme="amber"] .wrolpi-pagination button[data-active] {
.wrolpi-searchbox-control {
display: flex;
align-items: center;
gap: 8px;
/*
* One inset for the padding, the gap between children, and the negative margin that
* welds the clear button to the edge. They were three separate 8px literals that had to
* stay in lockstep: changing the padding alone would have pushed the clear button past
* the border, and changing the gap alone would have re-inset it.
*/
--searchbox-inset: 8px;
gap: var(--searchbox-inset);
background: var(--panel);
border: 1px solid var(--border);
padding: 0 8px;
padding: 0 var(--searchbox-inset);
}
.wrolpi-searchbox-control:focus-within {
@ -357,20 +364,26 @@ html[data-theme="amber"] .wrolpi-searchbox-control:focus-within {
* the field: measured on the QA Pi, zero gap to the input, zero to the outer edge, and the
* full 44px height.
*
* Negative margins rather than removing the control's padding: the padding is what holds the
* search icon and the text off the border, and only this one child should escape it. The
* left margin cancels the flex gap as well, so the divider lands against the text the way
* the attached button did.
* A negative right margin rather than removing the control's padding: the padding is what
* holds the search icon and the text off the border, and only this one child should escape
* it. The gap to its LEFT is deliberately left alone -- cancelling that too welded the
* loading spinner to the divider whenever a field was both `loading` and `clearable`, which
* SearchResultsInput does, and the spinner is not part of this control.
*
* `html` because Mantine sets ActionIcon's height from `--ai-size` on its own single class;
* at equal specificity the winner is whichever stylesheet loaded last, which is not a thing
* to rely on.
* to rely on. `min-height` has to go with it: stretch cannot shrink a box below its own
* minimum, so leaving that at `--ai-size` would mean this rule only half-owned the height it
* claims to set -- true today only because the field happens to be the taller of the two.
*
* The corner radius needs no rule: the theme zeroes every `--mantine-radius-*`, so there are
* no rounded corners to leave panel-coloured wedges against the square field.
*/
html .wrolpi-searchbox-clear {
align-self: stretch;
height: auto;
margin-left: -8px;
margin-right: -8px;
min-height: unset;
margin-right: calc(-1 * var(--searchbox-inset));
border-left: 1px solid var(--border);
}