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.