From feaca59ce5581eebc8e86c908f91ea5031a99425 Mon Sep 17 00:00:00 2001 From: Roland Date: Sun, 2 Aug 2026 17:13:28 -0600 Subject: [PATCH] ui: three button-layout defects, each with a different cause Reported together as "button groups", but they turned out to be three unrelated things. The file browser's footer buttons touched while the Channel Edit page's had gaps. Channel Edit lays its buttons out with Mantine's `Group`, which is a flex row with a gap; the footer is hand-rolled and was a plain block, so its inline-block buttons sat flush -- JSX drops whitespace-only lines, so there was not even a space character between them. The footer is now the flex row every other button group already is, wrapping because it carries eight controls. The Settings config table's Import and Save buttons had off-centre icons while Restore in the same row looked right. Restore is an IconButton -- an ActionIcon, which centres a lone glyph by construction. Import and Save are icon-only Buttons, and an icon on a Button goes into Mantine's `leftSection`, which carries a `margin-inline-end` to hold it off the label. With no label that margin is pure offset: measured at 8px left of centre. An icon with no label is now rendered as the button's content instead, so it centres. `Children.toArray` decides that, because it drops nulls and the file browser passes `{label('Delete')}` -- null whenever the bar is too narrow for words, which is exactly where the offset showed most. The search field's clear button sat inside the field rather than attached to it: the control is a bordered box with `padding: 0 8px`, leaving the button 9px short of the edge and 2px short of the height. Semantic welded it to the field -- measured on the QA Pi at zero gap and full height -- so it now stretches and negative margins cancel the padding and the flex gap. Negative margins rather than dropping the control's padding, because that padding is what holds the search icon and the text off the border; only this one child should escape it. The `html` prefix on that rule is load-bearing: Mantine sets ActionIcon's height from `--ai-size` on its own single class, and at equal specificity the winner is whichever stylesheet loaded last. Fourteen new cases. One correction worth recording: the height assertion first compared against the control's border box and failed on correct behaviour -- the button fills the CONTENT box, and the 1px above and below it is the field's own border, which it should sit inside rather than paint over. Each fix planted separately; the icon change reddens three, the footer one, the clear button one. Every case is paired with an inverse -- a labelled button must keep its icon on the left, and the field must still be padded around its other children. 1128 jest / 63 suites, 226 in ui-layout.cy.js, clean npm run build. Confirmed live: footer gaps 8px on one row, config glyphs at 0.0px from centre, clear button 1px from the field edge at the field's full inner height. --- app/src/App.css | 13 ++ app/src/components/ui/Button.tsx | 30 +++- app/src/components/ui/ui-layout.cy.js | 197 ++++++++++++++++++++++++++ app/src/components/ui/ui.css | 26 ++++ 4 files changed, 262 insertions(+), 4 deletions(-) diff --git a/app/src/App.css b/app/src/App.css index 6a8cc2c1..52a95e5a 100644 --- a/app/src/App.css +++ b/app/src/App.css @@ -597,6 +597,19 @@ table.ratio-table td.min-width { background-color: var(--panel); border-top: 1px solid var(--border); z-index: 100; + /* + * A row with gaps, like every other group of buttons in the app. + * + * This was a plain block, so the buttons -- inline-block elements with no whitespace + * between them, because JSX drops whitespace-only lines -- sat flush against each other + * in one unbroken slab. Everywhere else buttons are laid out with Mantine's `Group`, + * which is a flex row with a gap; the file browser's footer is hand-rolled and never got + * one. It wraps because the bar carries eight controls and a phone cannot hold them. + */ + display: flex; + align-items: center; + flex-wrap: wrap; + gap: 0.5em; } /* Drag selection visual feedback for FileBrowser */ diff --git a/app/src/components/ui/Button.tsx b/app/src/components/ui/Button.tsx index 4e3e3e43..2084d684 100644 --- a/app/src/components/ui/Button.tsx +++ b/app/src/components/ui/Button.tsx @@ -65,9 +65,29 @@ export const resolveSize = (size: T): T => typeof size === 'string' ? ((semanticSizes[size] ?? size) as T) : size; export const Button = forwardRef(( - {role, icon, iconAfter, className, ...props}, ref + {role, icon, iconAfter, className, children, ...props}, ref ) => { const fromRole = role ? roleProps[role] : undefined; + + /* + * An icon with no label is the button's CONTENT, not a leading icon. + * + * Mantine gives `leftSection` a `margin-inline-end` to hold it off the label. With no + * label that margin is pure offset: the glyph sits about 8px left of the button's centre, + * which is what the import and save buttons on the Settings config table looked like next + * to a correctly centred Restore -- Restore is an IconButton, which centres a lone glyph + * by construction. + * + * `React.Children.toArray` drops null, undefined and booleans, which matters because the + * file browser's footer passes `{label('Delete')}` and that is null whenever the bar is + * 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; + const soleIcon = !labelled && (icon || iconAfter) && !(icon && iconAfter) + ? renderIcon(icon || iconAfter) + : undefined; + return (( color={props.color ?? fromRole?.color} variant={props.variant ?? fromRole?.variant} className={[fromRole?.className, className].filter(Boolean).join(' ') || undefined} - leftSection={renderIcon(icon)} - rightSection={renderIcon(iconAfter)} + leftSection={soleIcon ? undefined : renderIcon(icon)} + rightSection={soleIcon ? undefined : renderIcon(iconAfter)} {...props} size={resolveSize(props.size)} - /> + > + {soleIcon ?? children} + }); Button.displayName = 'Button'; diff --git a/app/src/components/ui/ui-layout.cy.js b/app/src/components/ui/ui-layout.cy.js index 3ea01011..ab349dad 100644 --- a/app/src/components/ui/ui-layout.cy.js +++ b/app/src/components/ui/ui-layout.cy.js @@ -3044,3 +3044,200 @@ describe('a disabled button keeps its own colour', () => { }); }); }); + +describe('a button with an icon and no label centres the icon', () => { + /* + * Mantine's `leftSection` carries a `margin-inline-end` to hold the icon off the label. + * With no label that margin is pure offset, and the glyph sits about 8px left of the + * button's centre. + * + * The Settings config table is where it showed: Import and Save are icon-only Buttons and + * looked visibly off, while Restore in the same row looked right -- Restore is an + * IconButton, which centres a lone glyph by construction. Measured on the running app at + * -8px before the fix. + */ + it('puts the glyph in the middle when there is nothing else in the button', () => { + cy.mountUI(); + + cy.get('[data-testid="null-label"]').should(($button) => { + const box = $button[0].getBoundingClientRect(); + const glyph = $button[0].querySelector('svg').getBoundingClientRect(); + + expect((glyph.left + glyph.width / 2) - (box.left + box.width / 2), + 'glyph centre vs button centre').to.be.closeTo(0, 1); + }); + }); + + it('is a real constraint: a labelled button keeps its icon on the left', () => { + // The inverse. Centring the glyph must not apply when there IS a label, or every + // labelled button in the app loses its leading icon's position. + cy.mountUI(); + + cy.get('[data-testid="labelled"]').should(($button) => { + const box = $button[0].getBoundingClientRect(); + const glyph = $button[0].querySelector('svg').getBoundingClientRect(); + + expect((glyph.left + glyph.width / 2) - (box.left + box.width / 2), + 'the icon leads the label').to.be.lessThan(-4); + expect($button[0].textContent, 'and the label is still there').to.contain('Delete'); + }); + }); + + it('centres a trailing-only icon too', () => { + // `rightSection` has the mirror-image margin. + cy.mountUI( + + + ); + + cy.get('.sticky-footer').should(($footer) => { + const boxes = ['a', 'b', 'c'] + .map(id => $footer[0].querySelector(`[data-testid="${id}"]`).getBoundingClientRect()); + + expect(boxes[1].left - boxes[0].right, 'gap between the first two') + .to.be.greaterThan(2); + expect(boxes[2].left - boxes[1].right, 'gap between the next two') + .to.be.greaterThan(2); + }); + }); + + it('keeps them on one line and vertically aligned', () => { + // A flex row is only right if it behaves like one: the buttons share a baseline and + // do not stack while there is room for them. + cy.mountUI(
+ + +
); + + cy.get('.sticky-footer').should(($footer) => { + const a = $footer[0].querySelector('[data-testid="a"]').getBoundingClientRect(); + const b = $footer[0].querySelector('[data-testid="b"]').getBoundingClientRect(); + + expect(a.top, 'same row').to.be.closeTo(b.top, 1); + }); + }); +}); + +describe('the search field\'s clear button is attached to it', () => { + /* + * The control is a bordered box with `padding: 0 8px`, so the clear button sat 9px short + * of the right edge and 2px short of the full height -- a glyph dropped inside the input + * rather than a control of its own. + * + * Semantic rendered it as an action button welded to the field. Measured on the QA Pi, + * which still runs Semantic: zero gap from the input, zero to the outer edge, and the + * full height of the field. + */ + const mountField = () => cy.mountUI(
+ {}}/> +
); + + it('meets the right-hand edge of the field', () => { + mountField(); + + cy.get('.wrolpi-searchbox-clear').should(($clear) => { + const control = Cypress.$('.wrolpi-searchbox-control')[0].getBoundingClientRect(); + const clear = $clear[0].getBoundingClientRect(); + + expect(control.right - clear.right, 'gap to the field edge').to.be.closeTo(0, 1.5); + }); + }); + + it('fills the height of the field', () => { + /* + * Half the reason it read as "inside": a 36px control floating in a 38px box. + * + * Measured against the control's CONTENT box, not its border box. The field's own + * 1px border is the only thing left above and below the button, and the button + * should sit inside it rather than paint over it -- `clientHeight` is that box, and + * comparing against `getBoundingClientRect().height` would be demanding the button + * cover the field's border, which is a different and wrong design. + */ + mountField(); + + cy.get('.wrolpi-searchbox-clear').should(($clear) => { + const control = Cypress.$('.wrolpi-searchbox-control')[0]; + const border = parseFloat(getComputedStyle(control).borderTopWidth); + + expect(border, 'the field has a border to sit inside').to.be.greaterThan(0); + expect($clear[0].getBoundingClientRect().height, 'clear button height') + .to.be.closeTo(control.clientHeight, 1); + }); + }); + + it('is divided from the text rather than merged into it', () => { + // Attached is not the same as indistinguishable: without a rule between them the + // glyph reads as part of the input again, just further right. + mountField(); + + cy.get('.wrolpi-searchbox-clear').should(($clear) => { + const style = getComputedStyle($clear[0]); + + expect(parseFloat(style.borderLeftWidth), 'a divider on its left') + .to.be.greaterThan(0); + expect(style.borderLeftStyle).to.not.equal('none'); + }); + }); + + 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 + * been removed instead, the search icon and the text would sit against the border + * too -- so this checks the thing that must NOT have moved. + */ + mountField(); + + cy.get('.wrolpi-searchbox-icon').should(($icon) => { + const control = Cypress.$('.wrolpi-searchbox-control')[0].getBoundingClientRect(); + + expect($icon[0].getBoundingClientRect().left - control.left, 'icon is inset') + .to.be.greaterThan(4); + }); + }); +}); diff --git a/app/src/components/ui/ui.css b/app/src/components/ui/ui.css index b9beb121..27162c5b 100644 --- a/app/src/components/ui/ui.css +++ b/app/src/components/ui/ui.css @@ -348,6 +348,32 @@ html[data-theme="amber"] .wrolpi-searchbox-control:focus-within { flex: none; } +/* + * The clear button is attached to the field, not floating inside it. + * + * The control is a bordered box with `padding: 0 8px`, so the button sat 9px short of the + * right edge and 2px short of the full height -- reading as a glyph dropped inside the input + * rather than as a control of its own. Semantic rendered this as an action button welded to + * 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. + * + * `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. + */ +html .wrolpi-searchbox-clear { + align-self: stretch; + height: auto; + margin-left: -8px; + margin-right: -8px; + border-left: 1px solid var(--border); +} + /* * The suggestion list overlays the page rather than pushing it down — a search * that reflows the results underneath it is unusable while typing.