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(