From 2058246ef030ecffdb7af6d011e982e5160fd0cd Mon Sep 17 00:00:00 2001 From: Roland Date: Sun, 2 Aug 2026 14:25:04 -0600 Subject: [PATCH] ui: the navbar corner was nudged rather than laid out, and it showed Three faults in the bar, all reproduced by measurement before being fixed. The indicators sat low. The slot carried `marginTop: 0.8em`, a nudge left over from Semantic's menu; the corner centres its children, and a top margin is part of the margin box being centred, so it applied as a ~6px downward shift. It read as an error on the hotspot and the theme picker but not on share or search, which is why it survived: an IconButton is a square ActionIcon that centres its glyph exactly, while a bare-icon anchor sits high inside a line box that reserves descender space for text that is not there. The nudge cancelled that out on one shape and was pure error on the other. Both are laid out now -- flex, centred, no nudge -- and the anchors get `display: flex` so they hug their glyph instead of a text line. Help and Admin highlighted only a box around their text while Videos and Statistics highlighted the full height. Same class, same rules, different parent: the bar stretches its children, but the corner is a nested flex container that centres its own, so a link inside it was sized to its content. The hover background is painted on the link box, so that box IS the affordance. The bar broke onto a second line at certain widths. Two measurement errors that only overlap in a narrow band, which is why it appeared while resizing rather than always. useOverflowNav measured the WRAPPER's offsetWidth and subtracted only the home link and the corner, missing the bar's own `padding: 0 0.25em` -- about 8px it believed it had. And the hidden "More" placeholder it measured was a plain button with no dropdown caret, about 21px narrower than the button that eventually rendered. The placeholder is now the same component as the real trigger, so the two cannot drift again. Tested in ui-layout.cy.js against the real , which takes its colours, home link and indicators as props and so needs none of NavBar's polling hooks. Indicator centres are measured against the bar's centre rather than each other, because three things can be equally and consistently wrong and were. The wrap case sweeps 1180px down to 640px in 6px steps -- a spot check steps over a band that narrow. All three failed first: share 3.4px high, Help 24.8px against a 54.4px bar, and two rows at 1156px. Confirmed on the running app: every indicator at 0.0px from the bar's centre, Help and Admin at the full 54.4px, and no wrap anywhere from 1500px to 780px. 1115 jest / 62 suites, 182 in ui-layout.cy.js, clean npm run build. --- app/src/App.css | 38 ++++++ app/src/components/Nav.js | 45 +++++-- app/src/components/ui/ui-layout.cy.js | 185 ++++++++++++++++++++++++++ app/src/hooks/useOverflowNav.js | 20 ++- 4 files changed, 279 insertions(+), 9 deletions(-) diff --git a/app/src/App.css b/app/src/App.css index dde63798..6f5a98df 100644 --- a/app/src/App.css +++ b/app/src/App.css @@ -71,6 +71,44 @@ html .wrolpi-navbar .mantine-ActionIcon-root:hover { color: inherit; } +/* + * One status indicator's slot. Laid out rather than nudged: see NavIconWrapper in Nav.js + * for the 0.8em top margin this replaces and why it looked correct on half the indicators. + */ +.wrolpi-navbar-icon { + display: flex; + align-items: center; + margin-left: 1.5em; +} + +/* + * An anchor in the corner hugs its glyph instead of a text line box. + * + * A line box reserves descender space for text that is not there, which leaves an icon + * sitting about 3px above the centre of the box being centred -- the share and search + * glyphs, but not the hotspot or theme picker, which are square ActionIcons that centre + * their own glyph. That difference is what made a single top margin look right on some + * indicators and wrong on others. + */ +.wrolpi-navbar-right a { + display: flex; + align-items: center; +} + +/* + * A link in the corner fills the bar's height, as one outside it does. + * + * Help and Admin highlighted only a box around their text while Videos and Statistics + * highlighted the full height. Same class and same rules; the difference is the parent. + * The bar stretches its children, but the corner is a nested flex container that centres + * its own, so a link inside it was sized to its content. The hover background is painted + * on the link box, so this is the hover affordance itself and not just its outline. + */ +.wrolpi-navbar-right .wrolpi-navbar-link, +.wrolpi-navbar-right .wrolpi-navbar-link-button { + align-self: stretch; +} + .channel-edit { border-radius: 3px; padding: 0.75em; diff --git a/app/src/components/Nav.js b/app/src/components/Nav.js index d0a78733..9baba2da 100644 --- a/app/src/components/Nav.js +++ b/app/src/components/Nav.js @@ -102,15 +102,30 @@ function DropdownMenuItem({link}) { } +/* + * The trigger for a navbar dropdown. + * + * Shared with the hidden placeholder DesktopNav measures on its first render, and that is + * the point of extracting it. The placeholder used to be a plain `` + * with no caret, so the space reserved for the overflow menu was about 21px short of the + * button that eventually rendered -- the row overflowed by that much and the bar wrapped. + * Measuring one thing and drawing another is only ever right by coincidence. + */ +const NavDropdownTrigger = React.forwardRef(({text, ...props}, ref) => + +); +NavDropdownTrigger.displayName = 'NavDropdownTrigger'; + function DropdownLinks({link}) { // A labelled dropdown trigger: the desktop "More" overflow, or any nested // link group handed to MenuLink. return - + {link.links.map(l => )} @@ -130,9 +145,23 @@ function MobileMenu({links}) { } -function NavIconWrapper({children}) { +/* + * One status indicator's slot in the right-hand corner. + * + * This used to carry `marginTop: 0.8em`, a nudge left over from Semantic's menu. The corner + * centres its children, and a top margin is part of the margin box being centred, so the + * nudge pushed every indicator down by half of it -- about 6px. + * + * That read as an error on some and not others, which is why it survived: the bare-icon + * anchors (share, search) sit high inside their line box because it reserves descender space + * for text that is not there, so the nudge happened to cancel out. An IconButton is a square + * ActionIcon that centres its glyph exactly, so for the hotspot and the theme picker the + * nudge was 6px of pure error. Laying the slot out as a flex box centres both shapes on + * their real glyph box and needs no nudge at all. + */ +export function NavIconWrapper({children}) { if (children) { - return
{children}
+ return
{children}
} else { // Do not use navbar space if children is empty. return @@ -383,7 +412,7 @@ export function NavBar() { } -function DesktopNav({navColors, homeLink, icons}) { +export function DesktopNav({navColors, homeLink, icons}) { const containerRef = React.useRef(null); const homeRef = React.useRef(null); const rightMenuRef = React.useRef(null); @@ -410,7 +439,7 @@ function DesktopNav({navColors, homeLink, icons}) { ))} {!isReady && - + } {isReady && overflowLinks.length > 0 && } diff --git a/app/src/components/ui/ui-layout.cy.js b/app/src/components/ui/ui-layout.cy.js index 996a8ec5..9ac32c43 100644 --- a/app/src/components/ui/ui-layout.cy.js +++ b/app/src/components/ui/ui-layout.cy.js @@ -12,6 +12,7 @@ import {clearToasts, toast} from './toast'; import {monochromeThemes, themeNames} from '../../themes/names'; import {navColorNames} from '../../themes/navColors'; import {NavBarSample} from '../ThemeSamplePage'; +import {DesktopNav, NavIconWrapper} from '../Nav'; /* `--panel` is authored as a hex; computed backgrounds come back as rgb(). */ const hexToRgb = (hex) => { @@ -2218,3 +2219,187 @@ describe('the navigation bar is legible on every colour a user can pick', () => }); }); }); + +describe('the navigation bar corner', () => { + /* + * The real , which takes its colours, home link and indicators as props and + * so needs none of the polling hooks wires up. The indicators are the shapes + * the app actually puts there, in the real NavIconWrapper: a bare-icon anchor (share), an + * unwrapped anchor (search), and two IconButtons (hotspot, theme picker). Those three + * shapes take their geometry by three different routes, which is the whole problem. + */ + const indicators = <> + + {/* eslint-disable-next-line jsx-a11y/anchor-is-valid */} + + + + + } label='Hotspot'> + + }/> + + + + + ; + + const mountBar = (width, theme = 'light') => cy.mountUI( +
+ WROLPi} + icons={indicators} + /> +
, + {theme}, + ); + + const centreOf = (rect) => rect.top + rect.height / 2; + + it('centres every indicator on the bar, whatever shape it is', () => { + /* + * The bug this replaces was a 0.8em top margin on the slot, which the corner centres + * along with its content and so half-applied as a ~6px downward shift. It looked + * correct on the anchors and wrong on the two IconButtons, because an anchor's glyph + * already sits high inside a line box that reserves descender space. + * + * Measured against the bar's own centre rather than against each other: three things + * can be equally and consistently wrong, and were. + */ + mountBar(1400); + + cy.get('.wrolpi-navbar').should(($nav) => { + const bar = centreOf($nav[0].getBoundingClientRect()); + ['share', 'hotspot', 'theme'].forEach((name) => { + const glyph = $nav[0].querySelector(`[data-testid="${name}"] svg`); + expect(glyph, `${name} has a glyph`).to.not.be.null; + expect(centreOf(glyph.getBoundingClientRect()), `${name} vertical centre`) + .to.be.closeTo(bar, 1.5); + }); + }); + }); + + it('gives the right-hand links the same hit area as the left-hand ones', () => { + /* + * Help and Admin highlighted only a box around their text, while Videos and + * Statistics highlighted the full height of the bar. The corner is a nested flex + * container that centres its children, so a link inside it is sized to its content + * while a link that is a direct child of the bar is stretched by `align-items: + * stretch`. Same class, same rules, different parent. + * + * The hover background is painted on the link box, so this IS the hover affordance. + */ + mountBar(1400); + + cy.get('.wrolpi-navbar').should(($nav) => { + const bar = $nav[0].getBoundingClientRect().height; + const links = [...$nav[0].querySelectorAll('.wrolpi-navbar-link')]; + const corner = [...$nav[0] + .querySelectorAll('.wrolpi-navbar-right .wrolpi-navbar-link')]; + + // Help and Admin. If the corner has no links this passes on an empty list. + expect(corner.length, 'links in the corner').to.be.at.least(2); + expect(links.length, 'links in total').to.be.greaterThan(corner.length); + + corner.forEach((link) => { + expect(link.getBoundingClientRect().height, `${link.textContent} hit area`) + .to.be.closeTo(bar, 1); + }); + }); + }); + + it('is a real constraint: the left-hand links already filled the bar', () => { + // Without this the case above could pass by the bar having collapsed to the height + // of its text, which would be a different bug wearing the same number. + mountBar(1400); + + cy.get('.wrolpi-navbar').should(($nav) => { + const bar = $nav[0].getBoundingClientRect(); + const first = $nav[0].querySelector('.wrolpi-navbar-link'); + + expect(bar.height, 'the bar is taller than a line of text').to.be.greaterThan(40); + expect(first.getBoundingClientRect().height, 'a left-hand link fills it') + .to.be.closeTo(bar.height, 1); + }); + }); +}); + +describe('the navigation bar never breaks onto a second line', () => { + /* + * The bar wraps as a failsafe (`flex-wrap: wrap`) and useOverflowNav is supposed to make + * that unreachable by moving links into "More" before they overflow. At certain widths + * it did not, and the bar took two rows. + * + * The arithmetic was off by the bar's own horizontal padding: `recalculate` measured the + * WRAPPER div's offsetWidth and subtracted only the home link and the corner, so it + * believed it had ~8px more room than the row actually has. Any width where the last + * link overhangs by less than that got shown and wrapped -- a narrow band, which is why + * it appeared only "at certain widths" while resizing. + * + * Swept rather than spot-checked, because a band that narrow is precisely what a + * spot-check steps over. + */ + /* + * Hoisted, not written inline into `icons={...}`. The icon-name audit in ui.test.js + * reads every string literal inside such an expression as an icon name, so an inline + * `label='Theme' variant='subtle'` fails that test with two names nobody wrote. + */ + const themeIcon = + + ; + + const mountAt = (width) => cy.mountUI( +
+ WROLPi} + icons={themeIcon} + /> +
, + ); + + // Wide enough that every link fits, so this is the height of exactly one row. + let singleRow; + + before(() => { + mountAt(1600); + cy.get('.wrolpi-navbar').then(($nav) => { + singleRow = $nav[0].getBoundingClientRect().height; + expect(singleRow, 'a single row has a height to compare against') + .to.be.greaterThan(30); + }); + }); + + /* + * 6px steps across the range where links start moving into More. Fine enough to land + * inside an 8px band wherever it falls. + */ + const widths = []; + for (let width = 1180; width >= 640; width -= 6) widths.push(width); + + it('stays one row at every width from 1180 down to 640', () => { + widths.forEach((width) => { + mountAt(width); + cy.get('.wrolpi-navbar').should(($nav) => { + expect($nav[0].getBoundingClientRect().height, `bar height at ${width}px`) + .to.be.at.most(singleRow + 2); + }); + }); + }); + + it('keeps every visible link inside the bar rather than clipping it', () => { + // The failure mode a naive "just use nowrap" fix would introduce: no second row, + // but Admin hanging off the right edge where nothing can reach it. + [1180, 1000, 860, 720, 640].forEach((width) => { + mountAt(width); + cy.get('.wrolpi-navbar').should(($nav) => { + const bar = $nav[0].getBoundingClientRect(); + [...$nav[0].querySelectorAll('.wrolpi-navbar-link')].forEach((link) => { + expect(link.getBoundingClientRect().right, `${link.textContent} at ${width}px`) + .to.be.at.most(bar.right + 0.5); + }); + }); + }); + }); +}); diff --git a/app/src/hooks/useOverflowNav.js b/app/src/hooks/useOverflowNav.js index 0061f936..c2b5cdc0 100644 --- a/app/src/hooks/useOverflowNav.js +++ b/app/src/hooks/useOverflowNav.js @@ -15,7 +15,25 @@ export function useOverflowNav({links, containerRef, homeRef, rightMenuRef, more const container = containerRef.current; if (!container || itemWidths.current.length === 0) return; - const containerWidth = container.offsetWidth; + /* + * The width a row of links actually has, which is NOT the wrapper's. + * + * The bar carries `padding: 0 0.25em`, and this measured the wrapper and subtracted + * only the home link and the corner -- so it believed it had about 8px more room + * than the row does. Any width where the last link overhangs by less than that was + * kept in the bar, the bar wrapped, and the user got two rows. A narrow band, which + * is why it showed up only "at certain widths" while resizing rather than always. + * + * SAFETY covers subpixel rounding on top: item widths come from + * getBoundingClientRect and are fractional, while clientWidth is an integer. + */ + const SAFETY = 1; + const bar = container.firstElementChild || container; + const barStyle = window.getComputedStyle(bar); + const containerWidth = bar.clientWidth + - (parseFloat(barStyle.paddingLeft) || 0) + - (parseFloat(barStyle.paddingRight) || 0) + - SAFETY; const homeEl = homeRef.current?.firstElementChild || homeRef.current; const homeWidth = homeEl ? homeEl.getBoundingClientRect().width : 0; const rightWidth = rightMenuRef.current ? rightMenuRef.current.getBoundingClientRect().width : 0;