mirror of
https://github.com/lrnselfreliance/wrolpi.git
synced 2026-10-26 19:01:39 -04:00
ui: headings open their section, and keep their help icon on one line
Two things, and the second was a regression this migration caused.
A heading had `margin: 0 0 10px` -- no top margin at all -- so it sat flush
against whatever came before and read as part of it rather than as the start of
the next thing. On /more/statistics "Tags" touched the panel above it with a
0px gap. Now 22px above and 10px below, because a heading belongs to what
follows it.
`* + .wrolpi-header` rather than a blanket margin, for the same reason
StatisticGroup needed it: a heading that opens a Panel or a page is already
where it should be, and pushing it down just dents the top of its own container.
The help icon on every HelpHeader and InfoHeader had dropped to its own line --
"Root CA Certificate" in Settings is the clearest one. `.inline-header` set
`h1..h5 {display: inline-block}` so the heading and a sibling `<span>` shared a
row, which worked while these were a bare `<h2>` and stopped the moment they
gained a block wrapper: the `<label>`'s block child forced a break.
Fixed in the library rather than patched in App.css. `Header` takes an `after`
slot rendered inside the heading element, where the existing flex row already
handles alignment and spacing. Both wrappers pass their icon through it, the
`<label>` moves inside the heading so `for_` keeps associating the text with its
field (three call sites rely on it), and the `.inline-header` rules are deleted.
Fifteen call sites are fixed by fixing the two wrappers.
Tests: containment in jest, and the part jsdom cannot judge -- that the two
boxes actually share a line, and that the heading stays one row tall -- in a
browser. Planted both original shapes: the margin removed, a blanket margin
denting its container, and the control moved back outside the heading element.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Roland
parent
2ce7601ebb
commit
c18fe84783
8 changed files with 221 additions and 43 deletions
|
|
@ -247,30 +247,6 @@ textarea.otp {
|
|||
color: #333;
|
||||
}
|
||||
|
||||
.inline-header h1 {
|
||||
display: inline-block;
|
||||
}
|
||||
|
||||
.inline-header h2 {
|
||||
display: inline-block;
|
||||
}
|
||||
|
||||
.inline-header h3 {
|
||||
display: inline-block;
|
||||
}
|
||||
|
||||
.inline-header h4 {
|
||||
display: inline-block;
|
||||
}
|
||||
|
||||
.inline-header h5 {
|
||||
display: inline-block;
|
||||
}
|
||||
|
||||
.inline-header span {
|
||||
margin-left: 1em;
|
||||
}
|
||||
|
||||
.card-icon {
|
||||
min-height: 163px;
|
||||
text-align: center;
|
||||
|
|
|
|||
|
|
@ -810,14 +810,22 @@ export function InfoHeader({
|
|||
required = false,
|
||||
...props
|
||||
}) {
|
||||
return <div className='inline-header' {...props}>
|
||||
<label htmlFor={for_}>
|
||||
<Header as={headerSize}>{headerContent} {required && <RequiredAsterisk/>}</Header>
|
||||
</label>
|
||||
<span>
|
||||
<InfoPopup content={popupContent} iconSize={iconSize} icon={icon} position={popupPosition} {...popupProps}/>
|
||||
</span>
|
||||
</div>
|
||||
/*
|
||||
* The icon goes in the heading's own `after` slot. It used to be a sibling `<span>`
|
||||
* beside a `<label>`, held on one line by `.inline-header h1..h5 {display: inline-block}`
|
||||
* -- which worked while these headings were a bare `<h2>` and stopped working the moment
|
||||
* they gained a block wrapper: the label's block child forced a break and every help icon
|
||||
* in the app dropped to its own line.
|
||||
*
|
||||
* The label stays, now inside the heading, because `for_` associates the text with a
|
||||
* field on three of these call sites.
|
||||
*/
|
||||
return <Header as={headerSize} {...props} after={
|
||||
<InfoPopup content={popupContent} iconSize={iconSize} icon={icon} position={popupPosition}
|
||||
{...popupProps}/>
|
||||
}>
|
||||
<label htmlFor={for_}>{headerContent} {required && <RequiredAsterisk/>}</label>
|
||||
</Header>
|
||||
}
|
||||
|
||||
export function HelpModal({
|
||||
|
|
@ -856,14 +864,13 @@ export function HelpHeader({
|
|||
required = false,
|
||||
...props
|
||||
}) {
|
||||
return <div className='inline-header' {...props}>
|
||||
<label htmlFor={for_}>
|
||||
<Header as={headerSize}>{headerContent} {required && <RequiredAsterisk/>}</Header>
|
||||
</label>
|
||||
<span>
|
||||
<HelpModal helpPath={helpPath} iconSize={iconSize} icon={icon} title={helpTitle} {...helpModalProps}/>
|
||||
</span>
|
||||
</div>
|
||||
// See the note in InfoHeader: the icon belongs in the heading's row, not beside it.
|
||||
return <Header as={headerSize} {...props} after={
|
||||
<HelpModal helpPath={helpPath} iconSize={iconSize} icon={icon} title={helpTitle}
|
||||
{...helpModalProps}/>
|
||||
}>
|
||||
<label htmlFor={for_}>{headerContent} {required && <RequiredAsterisk/>}</label>
|
||||
</Header>
|
||||
}
|
||||
|
||||
// All the hardware toggles share this shape: a Toggle whose state comes from a
|
||||
|
|
|
|||
|
|
@ -2,8 +2,8 @@ import React from 'react';
|
|||
import {act, render, screen, waitFor} from '../test-utils';
|
||||
import userEvent from '@testing-library/user-event';
|
||||
import {
|
||||
contrastingColor, contrastRatio, DirectorySearch, loadRole, SearchResultsInput,
|
||||
TAG_TEXT_DARK, TAG_TEXT_LIGHT,
|
||||
contrastingColor, contrastRatio, DirectorySearch, HelpHeader, InfoHeader, loadRole,
|
||||
SearchResultsInput, TAG_TEXT_DARK, TAG_TEXT_LIGHT,
|
||||
} from './Common';
|
||||
import {healthRole} from './admin/ControllerPage';
|
||||
|
||||
|
|
@ -622,3 +622,49 @@ describe('healthRole', () => {
|
|||
expect(healthRole(value)).toBe('neutral'));
|
||||
});
|
||||
});
|
||||
|
||||
describe('a heading with a help icon keeps the icon on its own line', () => {
|
||||
/*
|
||||
* `HelpHeader` and `InfoHeader` put their icon in a sibling `<span>` beside a `<label>`,
|
||||
* held on one line by `.inline-header h1..h5 {display: inline-block}` in App.css. That
|
||||
* worked while the heading was a bare `<h2>`; the moment it gained a block wrapper the
|
||||
* label's block child forced a break, and every help icon in the app dropped below its
|
||||
* own heading -- visible on "Root CA Certificate" in Settings.
|
||||
*
|
||||
* They pass the icon through the heading's `after` slot now. Containment is the claim
|
||||
* jsdom can judge; that the two actually share a line is asserted in ui-layout.cy.js,
|
||||
* since jsdom has no layout at all.
|
||||
*/
|
||||
it('renders the help control inside the heading element', () => {
|
||||
render(<HelpHeader headerSize='h3' headerContent='Root CA Certificate'
|
||||
helpPath='/system/certificates/'/>);
|
||||
|
||||
const heading = screen.getByRole('heading', {name: /Root CA Certificate/});
|
||||
expect(heading).toContainElement(screen.getByRole('button', {name: 'Help'}));
|
||||
});
|
||||
|
||||
it('renders the info popup inside the heading element', () => {
|
||||
render(<InfoHeader headerSize='h4' headerContent='Video Resolutions'
|
||||
popupContent='Highest available is tried first.'/>);
|
||||
|
||||
const heading = screen.getByRole('heading', {name: /Video Resolutions/});
|
||||
expect(heading.querySelector('svg')).not.toBeNull();
|
||||
});
|
||||
|
||||
it('keeps the label association, which is what `for_` is for', () => {
|
||||
// Three call sites use it to tie the heading text to the field below it.
|
||||
render(<InfoHeader headerSize='h4' headerContent='Video Resolutions'
|
||||
for_='video_resolutions_input' popupContent='...'/>);
|
||||
|
||||
expect(screen.getByText(/Video Resolutions/).closest('label'))
|
||||
.toHaveAttribute('for', 'video_resolutions_input');
|
||||
});
|
||||
|
||||
it('leaves no .inline-header behind', () => {
|
||||
// The class and its stylesheet rules are gone; a stray one would be dead markup
|
||||
// relying on CSS that no longer exists.
|
||||
const {container} = render(<HelpHeader headerContent='Root CA Certificate' helpPath='/x/'/>);
|
||||
|
||||
expect(container.querySelector('.inline-header')).toBeNull();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -166,6 +166,14 @@ export function ThemeSamplePage() {
|
|||
<Header as='h3' icon='folder' dividing>With an icon and a divider (h3)</Header>
|
||||
<Header as='h4' subheader='And a subheader below it'>Subsection (h4)</Header>
|
||||
<Header as='h5'>Smallest (h5)</Header>
|
||||
{/*
|
||||
The `after` slot. A help button beside the text rather than under it --
|
||||
which is where every one of them sat until headings gained an `after`,
|
||||
because a sibling placed after the heading's block wrapper wraps.
|
||||
*/}
|
||||
<Header as='h3' after={<Icon name='question' size='small'/>}>
|
||||
With a help icon beside it
|
||||
</Header>
|
||||
<p style={{fontSize: 12, color: 'var(--muted)', marginBottom: 0}}>
|
||||
The level picks the size, so the type scale stays a scale: call sites
|
||||
choose a heading level for the document outline, never a font size.
|
||||
|
|
|
|||
|
|
@ -27,6 +27,14 @@ export interface HeaderProps extends Omit<React.HTMLAttributes<HTMLHeadingElemen
|
|||
* inline style to do this, which is how a hex ends up in the markup.
|
||||
*/
|
||||
color?: string;
|
||||
/**
|
||||
* Trailing content on the heading's own line — a help button, a count, a small
|
||||
* control. It goes inside the heading's flex row, which is the only place it can
|
||||
* sit beside the text: the wrapper is a block, so a sibling placed after it wraps
|
||||
* to the next line. That is exactly what happened to every help icon in the app
|
||||
* when these headings stopped being a bare `<h3>`.
|
||||
*/
|
||||
after?: React.ReactNode;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
@ -37,7 +45,7 @@ export interface HeaderProps extends Omit<React.HTMLAttributes<HTMLHeadingElemen
|
|||
* choosing — that is what keeps the type scale a scale.
|
||||
*/
|
||||
export function Header({
|
||||
as = 'h3', icon, subheader, dividing, color, className, style, children, ...props
|
||||
as = 'h3', icon, subheader, dividing, color, after, className, style, children, ...props
|
||||
}: HeaderProps) {
|
||||
const Tag = as;
|
||||
return <div className={['wrolpi-header', dividing ? 'wrolpi-header-dividing' : '', className]
|
||||
|
|
@ -51,6 +59,7 @@ export function Header({
|
|||
? <Icon name={icon} size='medium'/>
|
||||
: <Icon component={icon} size='medium'/>)}
|
||||
<span>{children}</span>
|
||||
{after && <span className='wrolpi-header-after'>{after}</span>}
|
||||
</Tag>
|
||||
{subheader && <div className='wrolpi-header-sub'>{subheader}</div>}
|
||||
</div>
|
||||
|
|
|
|||
|
|
@ -388,6 +388,94 @@ describe('a load reading ranks itself in the monochrome themes', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('a heading keeps its trailing control on its own line', () => {
|
||||
/*
|
||||
* The defect this fixes: `.wrolpi-header` is a block, so a help icon placed after it
|
||||
* wrapped to the next line -- every `HelpHeader` and `InfoHeader` in the app, most
|
||||
* visibly "Root CA Certificate" on the Settings page.
|
||||
*
|
||||
* A pure layout claim, so jsdom cannot judge it: getBoundingClientRect returns zeros
|
||||
* there, and containment alone (which the jest tests assert) does not prove two boxes
|
||||
* share a line.
|
||||
*/
|
||||
themeNames.forEach((theme) => {
|
||||
it(`sits the control beside the text in ${theme}`, () => {
|
||||
cy.mountUI(
|
||||
<Header as='h3' after={<Button role='cancel' size='xs'>Help</Button>}>
|
||||
Root CA Certificate
|
||||
</Header>,
|
||||
{theme},
|
||||
);
|
||||
|
||||
cy.get('.wrolpi-header-after').should(($after) => {
|
||||
const control = $after[0].getBoundingClientRect();
|
||||
const text = $after[0].previousElementSibling.getBoundingClientRect();
|
||||
|
||||
// Beside, not below: they overlap vertically and the control is to the right.
|
||||
expect(control.top, 'control starts above the text baseline')
|
||||
.to.be.lessThan(text.bottom);
|
||||
expect(text.top, 'text starts above the control baseline')
|
||||
.to.be.lessThan(control.bottom);
|
||||
expect(control.left, 'control is after the text').to.be.at.least(text.right);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
it('does not stretch the heading to make room', () => {
|
||||
// A wrapped control would make the heading two lines tall. One line is the claim.
|
||||
cy.mountUI(
|
||||
<Header as='h3' after={<Button role='cancel' size='xs'>Help</Button>}>
|
||||
Root CA Certificate
|
||||
</Header>,
|
||||
{theme: 'light'},
|
||||
);
|
||||
|
||||
cy.get('.wrolpi-header-text').should(($heading) => {
|
||||
const control = Cypress.$('.wrolpi-header-after')[0].getBoundingClientRect();
|
||||
expect($heading[0].getBoundingClientRect().height, 'heading is one row tall')
|
||||
.to.be.lessThan(control.height * 2);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('a heading opens a section rather than closing the last one', () => {
|
||||
it('leaves room above a heading that follows something', () => {
|
||||
/*
|
||||
* With no top margin a heading sat flush against the panel above it and read as part
|
||||
* of it -- "Tags" on /more/statistics touched the panel above with a 0px gap.
|
||||
*/
|
||||
cy.mountUI(
|
||||
<div>
|
||||
<Panel>Files</Panel>
|
||||
<Header as='h3'>Tags</Header>
|
||||
<Panel>34 tags</Panel>
|
||||
</div>,
|
||||
{theme: 'light'},
|
||||
);
|
||||
|
||||
cy.get('.wrolpi-header').should(($header) => {
|
||||
const panel = Cypress.$('.wrolpi-panel')[0].getBoundingClientRect();
|
||||
const gap = $header[0].getBoundingClientRect().top - panel.bottom;
|
||||
expect(gap, 'gap above the heading').to.be.at.least(12);
|
||||
|
||||
// And more above than below, because the heading belongs to what follows it.
|
||||
const below = Cypress.$('.wrolpi-panel')[1].getBoundingClientRect().top
|
||||
- $header[0].getBoundingClientRect().bottom;
|
||||
expect(gap, 'more air above the heading than below it').to.be.greaterThan(below);
|
||||
});
|
||||
});
|
||||
|
||||
it('does not dent the top of its own container', () => {
|
||||
// A blanket margin would push a heading that opens a Panel away from the Panel's edge.
|
||||
cy.mountUI(<Panel><Header as='h3'>Downloads</Header><p>body</p></Panel>, {theme: 'light'});
|
||||
|
||||
cy.get('.wrolpi-header').should(($header) => {
|
||||
expect(getComputedStyle($header[0]).marginTop, 'first child has no top margin')
|
||||
.to.equal('0px');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('a table header is a surface, not just bold text', () => {
|
||||
themeNames.forEach((theme) => {
|
||||
it(`separates the header from every body row in ${theme}`, () => {
|
||||
|
|
|
|||
|
|
@ -743,6 +743,30 @@ html[data-theme="night"] .wrolpi-tag::before {
|
|||
margin: 0 0 10px;
|
||||
}
|
||||
|
||||
/*
|
||||
* A heading opens a section, so it needs more air above it than below — it belongs to
|
||||
* what follows, not to what came before. With no top margin at all a heading sat flush
|
||||
* against the panel above it, which read as part of that panel rather than as the start
|
||||
* of the next thing (visible on /more/statistics, where "Tags" touched the panel above).
|
||||
*
|
||||
* `* +` rather than a blanket margin: a heading that opens a Panel or a page is already
|
||||
* where it should be, and pushing it down would just dent the top of its own container.
|
||||
*/
|
||||
* + .wrolpi-header {
|
||||
margin-top: 22px;
|
||||
}
|
||||
|
||||
/*
|
||||
* Trailing content keeps the heading's baseline but not its size: a help button rendered
|
||||
* at heading scale is a 26px glyph next to a 17px word.
|
||||
*/
|
||||
.wrolpi-header-after {
|
||||
display: inline-flex;
|
||||
align-items: center;
|
||||
font-size: 13px;
|
||||
font-weight: 400;
|
||||
}
|
||||
|
||||
.wrolpi-header-dividing {
|
||||
border-bottom: 1px solid var(--border);
|
||||
padding-bottom: 6px;
|
||||
|
|
|
|||
|
|
@ -346,6 +346,26 @@ describe('Header', () => {
|
|||
expect(container.querySelector('svg')).toBeInTheDocument();
|
||||
expect(screen.getByText('Paste this into the extension')).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it('puts trailing content inside the heading, not after its wrapper', () => {
|
||||
/*
|
||||
* The whole point of the slot. `.wrolpi-header` is a block, so anything placed after
|
||||
* it starts a new line -- which is what happened to every help icon in the app when
|
||||
* these headings stopped being a bare `<h3>`. Inside the heading element it joins
|
||||
* the existing flex row.
|
||||
*/
|
||||
const {container} = renderUI(
|
||||
<Header as='h3' after={<button type='button'>Help</button>}>Root CA Certificate</Header>);
|
||||
|
||||
const heading = container.querySelector('h3');
|
||||
expect(heading).toContainElement(screen.getByRole('button', {name: 'Help'}));
|
||||
});
|
||||
|
||||
it('renders nothing extra when there is no trailing content', () => {
|
||||
const {container} = renderUI(<Header as='h3'>Root CA Certificate</Header>);
|
||||
|
||||
expect(container.querySelector('.wrolpi-header-after')).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe('PathInput', () => {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue