rt/wrolpi
1
0
Fork 0
mirror of https://github.com/lrnselfreliance/wrolpi.git synced 2026-10-26 19:01:39 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions

ui: the navbar corner was nudged rather than laid out, and it showed

Browse source 
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 <DesktopNav/>, 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.
This commit is contained in:
Roland 2026-08-02 14:25:04 -06:00 • committed by lrnselfreliance
parent 149b40acbd
commit 2058246ef0
4 changed files with 279 additions and 9 deletions
Download patch file Download diff file Expand all files Collapse all files

38
app/src/App.css
View file

@ -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;

45
app/src/components/Nav.js
View file

@ -102,15 +102,30 @@ function DropdownMenuItem({link}) {
</Menu.Item>
}
/*
* 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 `<button>More</button>`
* 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) =>
<button type='button' ref={ref} className='wrolpi-navbar-link wrolpi-navbar-link-button'
{...props}>
{text}
<Icon name='dropdown' size='small' style={{marginLeft: '0.35em'}}/>
</button>
);
NavDropdownTrigger.displayName = 'NavDropdownTrigger';
function DropdownLinks({link}) {
// A labelled dropdown trigger: the desktop "More" overflow, or any nested
// link group handed to MenuLink.
return <Menu position='bottom-end' withinPortal>
<Menu.Target>
<button type='button' className='wrolpi-navbar-link wrolpi-navbar-link-button'>
{link.text}
<Icon name='dropdown' size='small' style={{marginLeft: '0.35em'}}/>
</button>
<NavDropdownTrigger text={link.text}/>
</Menu.Target>
<Menu.Dropdown>
{link.links.map(l => <DropdownMenuItem key={l.key} link={l}/>)}
@ -130,9 +145,23 @@ function MobileMenu({links}) {
</Menu>
}
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 <div style={{marginTop: '0.8em', marginLeft: '1.5em'}}>{children}</div>
return <div className='wrolpi-navbar-icon'>{children}</div>
} else {
// Do not use navbar space if children is empty.
return <React.Fragment/>
@ -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}) {
</span>
))}
{!isReady && <span ref={moreRef} style={{visibility: 'hidden', flexShrink: 0}}>
<button type='button' className='wrolpi-navbar-link wrolpi-navbar-link-button'>More</button>
<NavDropdownTrigger text='More'/>
</span>}
{isReady && overflowLinks.length > 0 &&
<DropdownLinks link={{text: 'More', key: 'more', links: overflowLinks}}/>}

185
app/src/components/ui/ui-layout.cy.js
View file

@ -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 <DesktopNav/>, which takes its colours, home link and indicators as props and
* so needs none of the polling hooks <NavBar/> 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 = <>
<NavIconWrapper>
{/* eslint-disable-next-line jsx-a11y/anchor-is-valid */}
<a href='#' data-testid='share'><Icon name='share' size='large'/></a>
</NavIconWrapper>
<NavIconWrapper>
<IconButton data-testid='hotspot' label='Hotspot' variant='subtle' icon={() =>
<IconStack corner={<Icon name='x' size={12}/>} label='Hotspot'>
<Icon name='wifi' size='large'/>
</IconStack>}/>
</NavIconWrapper>
<NavIconWrapper>
<IconButton data-testid='theme' icon='sun' label='Theme' variant='subtle'/>
</NavIconWrapper>
</>;
const mountBar = (width, theme = 'light') => cy.mountUI(
<MemoryRouter><div style={{width}}>
<DesktopNav
navColors={{background: '#5c4fa8', color: '#ffffff', ratio: 6.69}}
homeLink={<a className='wrolpi-navbar-link' href='#'><i>WROLPi</i></a>}
icons={indicators}
/>
</div></MemoryRouter>,
{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 = <NavIconWrapper>
<IconButton icon='sun' label='Theme' variant='subtle'/>
</NavIconWrapper>;
const mountAt = (width) => cy.mountUI(
<MemoryRouter><div style={{width}}>
<DesktopNav
navColors={{background: '#5c4fa8', color: '#ffffff', ratio: 6.69}}
homeLink={<a className='wrolpi-navbar-link' href='#'><i>WROLPi</i></a>}
icons={themeIcon}
/>
</div></MemoryRouter>,
);
// 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);
});
});
});
});
});

20
app/src/hooks/useOverflowNav.js
View file

@ -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;