From 83dc2ac82fc298fba2c50f666ff631929a392bef Mon Sep 17 00:00:00 2001 From: Roland Date: Sun, 2 Aug 2026 17:28:46 -0600 Subject: [PATCH] review: one inset, and two tests that could not fail 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. --- app/src/components/ui/Button.tsx | 15 +++- app/src/components/ui/ui-layout.cy.js | 117 ++++++++++++++++++++++++++ app/src/components/ui/ui.css | 31 +++++-- 3 files changed, 153 insertions(+), 10 deletions(-) diff --git a/app/src/components/ui/Button.tsx b/app/src/components/ui/Button.tsx index 2084d684..ff9375ca 100644 --- a/app/src/components/ui/Button.tsx +++ b/app/src/components/ui/Button.tsx @@ -83,7 +83,20 @@ export const Button = forwardRef(( * 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; diff --git a/app/src/components/ui/ui-layout.cy.js b/app/src/components/ui/ui-layout.cy.js index ab349dad..321c5113 100644 --- a/app/src/components/ui/ui-layout.cy.js +++ b/app/src/components/ui/ui-layout.cy.js @@ -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(
+ + +
); + + 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(
+ + +
); + + 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(