mirror of
https://github.com/lrnselfreliance/wrolpi.git
synced 2026-10-26 19:01:39 -04:00
review: finish the sweep the first pass claimed
The review was right that completeness was not done. Every finding held up.
Three more pages mounted a `PageContainer` inside a route that already
provides page chrome -- `ManageMap`, `MapPins`, `ManageZim` -- so they sat a
further 1em down and 1em in from the tab bar than the viewers beside them.
The same defect as ArchiveSettings, in a shape the scan could not see: it
keyed on `PageContainer` route wrappers, and Map and Zim wrap their content
in a `wrolpi-stack` div instead. It now locates routers first and asks
whether the enclosing return provides chrome either way, resolves
`element={<ErrorBoundary><Page/></ErrorBoundary>}` as well as the bare form,
and knows about `class` components -- `ManageZim` is one, and a scan that
only knew functions reported it clean.
Five top-level blocks still added their own margin to the stack's gap: the
dashboard search row (52.8px against 17.6px, on the most-looked-at page in
the app), the Playlists filter row, both Docs filter rows, and the Map pins
filter. The extension page's hint panel had the same compound against a
Mantine Stack's gap.
ControllerPage held its four panel sections in a plain wrapper, so the
admin page's stack saw one child and the panels kept sharing edges -- the
exact hole Settings had.
The dashboard was reported clean in the first pass and was not. The sweep
measured wrapper-to-wrapper gaps, and that row's margin is INSIDE a Media
wrapper, so it never appeared: the measurement, not the page, was at fault.
Cypress gap assertions now derive from the stack's own `row-gap` instead of a
12-28px range that assumed the default interface scale and would have
false-failed a correct layout at a larger one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Roland
parent
85eeb1cd8d
commit
bb14bf4074
9 changed files with 125 additions and 65 deletions
|
|
@ -452,7 +452,13 @@ export function DashboardPage() {
|
|||
if (big) {
|
||||
inputProps.size = 'big';
|
||||
}
|
||||
return <div style={{display: 'flex', alignItems: 'center', gap: '0.5em', marginBottom: '2em'}}>
|
||||
/*
|
||||
* No `marginBottom`. This row is a top-level block of the dashboard, so its own 2em was
|
||||
* added to the page stack's gap -- 52.8px below the search box against 17.6px between
|
||||
* every other pair of blocks, on the most-looked-at page in the app. One source of
|
||||
* spacing, and it is the stack.
|
||||
*/
|
||||
return <div style={{display: 'flex', alignItems: 'center', gap: '0.5em'}}>
|
||||
{getSearchResultsInput(inputProps)}
|
||||
<SearchFilterButton fileFilterOptions={fileMimetypeFilterOptions} showDates={true} showDeep={true}
|
||||
size={big ? 'big' : undefined}/>
|
||||
|
|
|
|||
|
|
@ -727,7 +727,7 @@ describe('PageContainer', () => {
|
|||
stacks.forEach(stack => expect(stack.textContent).toBe('Body'));
|
||||
});
|
||||
|
||||
it('is never rendered by a page that a PageContainer already wraps', () => {
|
||||
it('is never rendered by a page whose route already provides page chrome', () => {
|
||||
/*
|
||||
* `PageContainer` carries `margin-top: 1em` and `padding: 1em`, so nesting one inside
|
||||
* another applies both twice: the page is pushed 1em further down and inset 1em further
|
||||
|
|
@ -736,10 +736,21 @@ describe('PageContainer', () => {
|
|||
* in another -- and read as "more margin than /archives/domains", which is exactly what
|
||||
* it was.
|
||||
*
|
||||
* Nothing about a doubled margin throws, so this is a source scan. It reads each route
|
||||
* wrapper (a `PageContainer` holding `<Routes>` or `<Outlet/>`), takes the components it
|
||||
* routes to, and checks the ones defined in the same file -- which is where a route and
|
||||
* its pages normally live together.
|
||||
* Nothing about a doubled margin throws, so this is a source scan. It finds each router
|
||||
* whose enclosing `return` provides page chrome, takes the components it routes to, and
|
||||
* checks the ones declared in the same file -- which is where a route and its pages
|
||||
* normally live together.
|
||||
*
|
||||
* "Chrome" is `PageContainer` OR a `wrolpi-stack` wrapper, not just the former. Keying on
|
||||
* `PageContainer` alone missed `MapRoute` and `ZimRoute`, which wrap their own content in a
|
||||
* stack instead -- and three pages under them (`ManageMap`, `MapPins`, `ManageZim`) mounted
|
||||
* a `PageContainer` each, so they sat a further 1em down and 1em in from the tab bar than
|
||||
* the viewers beside them. The same defect as Archive, in a shape the first scan could not
|
||||
* see.
|
||||
*
|
||||
* Known limit: a layout that routes through `<Outlet/>` has its `<Route>` list in App.js,
|
||||
* naming components from other files, so the page cannot be resolved from one file's text.
|
||||
* That is `VideosTabLayout`, whose pages are clean today but are not covered here.
|
||||
*/
|
||||
const src = path.join(__dirname, '..');
|
||||
const files = (dir) => fs.readdirSync(dir, {withFileTypes: true}).flatMap(entry => {
|
||||
|
|
@ -753,24 +764,9 @@ describe('PageContainer', () => {
|
|||
const offenders = [];
|
||||
for (const file of files(src)) {
|
||||
const text = fs.readFileSync(file, 'utf8');
|
||||
|
||||
/*
|
||||
* EVERY `<PageContainer>` in the file, not just the first. Checking only the first
|
||||
* is what let this bug through a version of this test: in Archive.js the first one
|
||||
* is `ArchiveSettingsPage`'s -- the offender -- and its block holds no `<Routes>`, so
|
||||
* the file was skipped before the route wrapper 400 lines below was ever looked at.
|
||||
*/
|
||||
for (const block of routeWrapperBlocks(text)) {
|
||||
const routed = [...block.matchAll(/element=\{<(\w+)\s*\/>\}/g)].map(m => m[1]);
|
||||
for (const name of routed) {
|
||||
// The component's body, bounded by the next top-level declaration.
|
||||
const declaration = new RegExp(`^(?:export\\s+)?(?:function|const)\\s+${name}\\b`, 'm');
|
||||
const found = declaration.exec(text);
|
||||
if (!found) continue; // Defined elsewhere; out of this scan's reach.
|
||||
const after = text.slice(found.index + 1);
|
||||
const next = after.search(/^(?:export\s+)?(?:function|const|class)\s/m);
|
||||
const body = next === -1 ? after : after.slice(0, next);
|
||||
if (body.includes('<PageContainer>')) {
|
||||
for (const block of chromedRouterBlocks(text)) {
|
||||
for (const name of routedComponents(block)) {
|
||||
if (bodyOf(text, name).includes('<PageContainer>')) {
|
||||
offenders.push(`${path.relative(src, file)}: ${name}`);
|
||||
}
|
||||
}
|
||||
|
|
@ -778,33 +774,64 @@ describe('PageContainer', () => {
|
|||
}
|
||||
|
||||
/*
|
||||
* The premise: a scan that found no route wrappers, or routed to no components, would
|
||||
* report no offenders and read exactly like a clean codebase.
|
||||
* The premise: a scan that found no routers, or resolved no page components, would report
|
||||
* no offenders and read exactly like a clean codebase.
|
||||
*/
|
||||
const wrappers = files(src).flatMap(file => routeWrapperBlocks(fs.readFileSync(file, 'utf8')));
|
||||
expect(wrappers.length).toBeGreaterThan(2);
|
||||
expect(wrappers.flatMap(b => [...b.matchAll(/element=\{<(\w+)\s*\/>\}/g)]).length)
|
||||
.toBeGreaterThan(8);
|
||||
const blocks = files(src).flatMap(file => chromedRouterBlocks(fs.readFileSync(file, 'utf8')));
|
||||
expect(blocks.length).toBeGreaterThan(3);
|
||||
expect(blocks.flatMap(routedComponents).length).toBeGreaterThan(8);
|
||||
|
||||
expect(offenders).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
/**
|
||||
* Every `<PageContainer>...</PageContainer>` in `text` that wraps a router -- so, the ones that
|
||||
* wrap a whole section of the app rather than one page. All of them, because a file may hold both
|
||||
* a route wrapper and the pages it routes to, in either order.
|
||||
* Every `<Routes>...</Routes>` in `text` whose enclosing `return` provides page chrome -- a
|
||||
* `PageContainer` or a `wrolpi-stack` wrapper. Those are the routers that have already spaced and
|
||||
* inset the page, so a page under one must not do it again.
|
||||
*
|
||||
* All of them, and located from the router rather than from the chrome: keying on the first
|
||||
* `<PageContainer>` in a file missed this entirely in Archive.js, where the first one belongs to
|
||||
* the offending page and holds no router at all.
|
||||
*/
|
||||
const routeWrapperBlocks = (text) => {
|
||||
const chromedRouterBlocks = (text) => {
|
||||
const blocks = [];
|
||||
let from = 0;
|
||||
for (;;) {
|
||||
const start = text.indexOf('<PageContainer>', from);
|
||||
const start = text.indexOf('<Routes>', from);
|
||||
if (start === -1) return blocks;
|
||||
const end = text.indexOf('</PageContainer>', start);
|
||||
const end = text.indexOf('</Routes>', start);
|
||||
if (end === -1) return blocks;
|
||||
const block = text.slice(start, end);
|
||||
if (/<Routes>|<Outlet\s*\/>/.test(block)) blocks.push(block);
|
||||
from = end + 1;
|
||||
|
||||
const returned = text.lastIndexOf('return', start);
|
||||
if (returned === -1) continue;
|
||||
const chrome = text.slice(returned, start);
|
||||
if (chrome.includes('<PageContainer>') || chrome.includes('wrolpi-stack')) {
|
||||
blocks.push(text.slice(start, end));
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
/**
|
||||
* The page components a router block routes to. Any capitalised tag that is not routing furniture,
|
||||
* so `element={<ErrorBoundary><Page/></ErrorBoundary>}` yields `Page` as well as the plain
|
||||
* `element={<Page/>}` form.
|
||||
*/
|
||||
const FURNITURE = new Set(['Route', 'Routes', 'ErrorBoundary', 'Suspense', 'Navigate', 'Outlet']);
|
||||
const routedComponents = (block) => [...new Set(
|
||||
[...block.matchAll(/<([A-Z]\w*)/g)].map(m => m[1]).filter(name => !FURNITURE.has(name)))];
|
||||
|
||||
/**
|
||||
* The source of the component named `name`, bounded by the next top-level declaration. `class` is
|
||||
* in both patterns because `ManageZim` is one, and a scan that only knew about functions would
|
||||
* have reported it clean.
|
||||
*/
|
||||
const bodyOf = (text, name) => {
|
||||
const declaration = new RegExp(`^(?:export\\s+)?(?:function|const|class)\\s+${name}\\b`, 'm');
|
||||
const found = declaration.exec(text);
|
||||
if (!found) return ''; // Declared elsewhere; out of this scan's reach.
|
||||
const after = text.slice(found.index + 1);
|
||||
const next = after.search(/^(?:export\s+)?(?:function|const|class)\s/m);
|
||||
return next === -1 ? after : after.slice(0, next);
|
||||
};
|
||||
|
|
|
|||
|
|
@ -469,7 +469,8 @@ function AuthorsPage() {
|
|||
const [searchStr, setSearchStr] = useOneQuery('author');
|
||||
const searchInputRef = React.useRef();
|
||||
|
||||
const header = <div style={{marginBottom: '1em'}}>
|
||||
// No `marginBottom`: a top-level block of the page, spaced by the page's stack.
|
||||
const header = <div>
|
||||
<SearchInput
|
||||
placeholder='Author filter...'
|
||||
size='large'
|
||||
|
|
@ -501,7 +502,8 @@ function SubjectsPage() {
|
|||
const [searchStr, setSearchStr] = useOneQuery('subject');
|
||||
const searchInputRef = React.useRef();
|
||||
|
||||
const header = <div style={{marginBottom: '1em'}}>
|
||||
// No `marginBottom`: a top-level block of the page, spaced by the page's stack.
|
||||
const header = <div>
|
||||
<SearchInput
|
||||
placeholder='Subject filter...'
|
||||
size='large'
|
||||
|
|
|
|||
|
|
@ -5,7 +5,6 @@ import {
|
|||
HandPointMessage,
|
||||
humanFileSize,
|
||||
InfoMessage,
|
||||
PageContainer,
|
||||
TabLinks,
|
||||
useTitle,
|
||||
WROLModeMessage
|
||||
|
|
@ -366,13 +365,18 @@ function ManageMap() {
|
|||
};
|
||||
|
||||
if (files === undefined) {
|
||||
return <PageContainer>
|
||||
return <>
|
||||
<WROLModeMessage content='Cannot modify Map'/>
|
||||
<ErrorMessage>Could not fetch map files</ErrorMessage>
|
||||
</PageContainer>;
|
||||
</>;
|
||||
}
|
||||
|
||||
return <PageContainer>
|
||||
/*
|
||||
* A fragment, not a `PageContainer`. `MapRoute` already provides the page chrome -- a
|
||||
* `wrolpi-stack` wrapper with its own top margin -- so a second one put this page a further
|
||||
* 1em down and 1em in from the tab bar than the viewer beside it.
|
||||
*/
|
||||
return <>
|
||||
<WROLModeMessage content='Cannot modify Map'/>
|
||||
|
||||
<Header as='h3'>Map Files</Header>
|
||||
|
|
@ -420,7 +424,7 @@ function ManageMap() {
|
|||
<HandPointMessage>
|
||||
<p>You can also view the map at <a href={MAP_VIEWER_URI}>{MAP_VIEWER_URI}</a></p>
|
||||
</HandPointMessage>
|
||||
</PageContainer>
|
||||
</>
|
||||
}
|
||||
|
||||
const PIN_COLORS = ["red", "blue", "green", "yellow", "orange", "purple"];
|
||||
|
|
@ -605,13 +609,15 @@ function MapPins() {
|
|||
)
|
||||
: pins;
|
||||
|
||||
return <PageContainer>
|
||||
// A fragment for the same reason as ManageMap above. The filter carries no `marginBottom`
|
||||
// either: it is a top-level block of the page, and the route's stack spaces it.
|
||||
return <>
|
||||
<TextInput
|
||||
leftSection={<Icon name='search'/>}
|
||||
placeholder='Filter by label or coordinates...'
|
||||
value={filter}
|
||||
onChange={(e) => setFilter(e.target.value)}
|
||||
style={{marginBottom: '1em', width: '100%'}}
|
||||
style={{width: '100%'}}
|
||||
/>
|
||||
<Media at='mobile'>
|
||||
<SortableTable
|
||||
|
|
@ -633,7 +639,7 @@ function MapPins() {
|
|||
emptyRow={emptyRow}
|
||||
/>
|
||||
</Media>
|
||||
</PageContainer>;
|
||||
</>;
|
||||
}
|
||||
|
||||
function MapPage() {
|
||||
|
|
|
|||
|
|
@ -128,7 +128,9 @@ export function PlaylistsPage() {
|
|||
}
|
||||
};
|
||||
|
||||
const header = <Group justify='space-between' align='flex-end' wrap='wrap' mb='1em'>
|
||||
// No `mb`: this is a top-level block of the page, and the page's stack spaces it. Its own
|
||||
// margin was added to that gap -- the same fix as the twin filter rows on Channels/Domains.
|
||||
const header = <Group justify='space-between' align='flex-end' wrap='wrap'>
|
||||
<SearchInput
|
||||
placeholder='Name filter...'
|
||||
size='large'
|
||||
|
|
|
|||
|
|
@ -9,7 +9,6 @@ import {
|
|||
IframeViewer,
|
||||
InfoMessage,
|
||||
normalizeEstimate,
|
||||
PageContainer,
|
||||
Paginator,
|
||||
TabLinks,
|
||||
TagIcon,
|
||||
|
|
@ -489,7 +488,12 @@ class ManageZim extends React.Component {
|
|||
kiwixCatalog = <ErrorMessage>Could not fetch catalog</ErrorMessage>;
|
||||
}
|
||||
|
||||
return <PageContainer>
|
||||
/*
|
||||
* A fragment, not a `PageContainer`. `ZimRoute` already provides the page chrome -- a
|
||||
* `wrolpi-stack` wrapper with its own top margin -- so a second one put this page a
|
||||
* further 1em down and 1em in from the tab bar than the viewer beside it.
|
||||
*/
|
||||
return <>
|
||||
<Header as='h2'>Zim Files</Header>
|
||||
{zimFilesBody}
|
||||
|
||||
|
|
@ -500,7 +504,7 @@ class ManageZim extends React.Component {
|
|||
|
||||
<DownloadMessage/>
|
||||
<ViewerMessage/>
|
||||
</PageContainer>
|
||||
</>
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1631,7 +1631,9 @@ function AdminControlsSection() {
|
|||
|
||||
export function ControllerPage() {
|
||||
return (
|
||||
<div>
|
||||
// `wrolpi-stack`: these four sections are panels held in a wrapper of this page's own,
|
||||
// so the stack on PageContainer does not reach them -- the same hole Settings had.
|
||||
<div className='wrolpi-stack'>
|
||||
<AdminControlsSection/>
|
||||
<ServicesSection/>
|
||||
<SambaSection/>
|
||||
|
|
|
|||
|
|
@ -32,7 +32,9 @@ function DestinationHint() {
|
|||
// Fall through silently — the user can still copy manually.
|
||||
}
|
||||
};
|
||||
return <Panel style={{marginBottom: '1em'}}>
|
||||
// No `marginBottom`: this Panel sits in a Mantine `Stack`, whose gap already spaces it.
|
||||
// A margin here is added to that gap -- the compound the panel-margin approach caused.
|
||||
return <Panel>
|
||||
<Header as='h4' icon='globe'>This WROLPi's URL</Header>
|
||||
<p style={{color: 'var(--muted)'}}>
|
||||
After installing the extension, paste this URL into the
|
||||
|
|
|
|||
|
|
@ -142,15 +142,19 @@ describe('a page stacks its own blocks', () => {
|
|||
<Panel>Calculators</Panel>
|
||||
</div>);
|
||||
|
||||
cy.get('.wrolpi-panel').should(($panels) => {
|
||||
const gaps = gapsBetween($panels);
|
||||
cy.get('.wrolpi-stack').should(($stack) => {
|
||||
/*
|
||||
* Measured against the stack's own `row-gap` rather than a pixel range. The range was
|
||||
* 12-28px, which assumes the default `--ui-scale`: at a larger scale the gap grows past
|
||||
* 28 and a correct layout would have failed the assertion. A separate test covers the
|
||||
* ratio; this one is about the panels really being that far apart.
|
||||
*/
|
||||
const declared = parseFloat(getComputedStyle($stack[0]).rowGap);
|
||||
expect(declared, 'the stack declares a gap').to.be.greaterThan(0);
|
||||
|
||||
const gaps = gapsBetween($stack[0].querySelectorAll('.wrolpi-panel'));
|
||||
expect(gaps, 'both pairs measured').to.have.length(2);
|
||||
gaps.forEach((gap) => {
|
||||
// Enough to read as a seam rather than a heavier border ...
|
||||
expect(gap, 'gap between panels').to.be.at.least(12);
|
||||
// ... and not so much that one page reads as unrelated cards.
|
||||
expect(gap, 'gap between panels').to.be.at.most(28);
|
||||
});
|
||||
gaps.forEach((gap) => expect(gap, 'gap between panels').to.be.closeTo(declared, 0.5));
|
||||
});
|
||||
});
|
||||
|
||||
|
|
@ -243,9 +247,14 @@ describe('a page stacks its own blocks', () => {
|
|||
<Panel>Also shown</Panel>
|
||||
</div>);
|
||||
|
||||
cy.get('.wrolpi-panel:visible').should(($panels) => {
|
||||
expect($panels, 'the hidden panel is not measured').to.have.length(2);
|
||||
expect(gapsBetween($panels)[0], 'one gap, not two').to.be.at.most(28);
|
||||
// One gap, not two: measured against the declared gap rather than a pixel ceiling, which
|
||||
// would have false-failed at a larger interface scale instead of catching a doubled gap.
|
||||
cy.get('.wrolpi-stack').should(($stack) => {
|
||||
const declared = parseFloat(getComputedStyle($stack[0]).rowGap);
|
||||
const shown = $stack[0].querySelectorAll('.wrolpi-panel:not([style*="none"])');
|
||||
const visible = [...shown].filter((el) => el.offsetParent !== null);
|
||||
expect(visible, 'the hidden panel is not measured').to.have.length(2);
|
||||
expect(gapsBetween(visible)[0], 'one gap, not two').to.be.closeTo(declared, 0.5);
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue