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: stop /archives/settings wrapping itself in a second PageContainer

Browse source 
`ArchiveRoute` wraps its `<Routes>` in a PageContainer, and
`ArchiveSettingsPage` wrapped itself in another.  PageContainer carries
`margin-top: 1em` and `padding: 1em`, so that page sat 1em lower and 1em
further in on all four sides than /archives/domains beside it -- which is
exactly what "seems to have more margin" was.  It is a fragment now, so its
panels are blocks of the route's own stack.

Guarded by a source scan rather than a render, since a doubled margin throws
nothing: it takes each PageContainer that wraps a router, reads the
components it routes to, and fails if one of them renders a PageContainer of
its own.

The first version of that scan passed against the live bug.  It looked at
only the first `<PageContainer>` in a file, and in Archive.js that is the
offender itself, whose block holds no `<Routes>` -- so the file was skipped
before the route wrapper 400 lines below was reached.  It reads every
occurrence now, and the premise assertions count both the wrappers found and
the routes within them.

Two more top-level blocks gave up margins that were being added to the
page's gap, both headers "matching" each other's pattern: the domain filter
row (35.2px against 17.6px) and the channel filter row.

/archives/settings and /archives/domains now measure identically -- tab bar
bottom at 136.8px, first block 17.6px below it, content inset 17.6px -- as
do the statistics and settings pages of every other tab group.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Roland 2026-08-05 15:53:04 -06:00
parent 52ae95ff47
commit 85eeb1cd8d
No known key found for this signature in database
GPG key ID: 1BA620CE6C9A4680
3 changed files with 97 additions and 6 deletions
Download patch file Download diff file Expand all files Collapse all files

15
app/src/components/Archive.js
View file

@ -498,8 +498,9 @@ export function DomainsPage() {
}
}, {enableOnFormTags: false});
// Header section matching ChannelsPage pattern
const header = <div style={{marginBottom: '1em', display: 'flex', flexWrap: 'wrap', gap: '1em', alignItems: 'center'}}>
// Header section matching ChannelsPage pattern. No `marginBottom`: this is a top-level block
// of the page, and the page's stack spaces it -- its own margin was added to that gap.
const header = <div style={{display: 'flex', flexWrap: 'wrap', gap: '1em', alignItems: 'center'}}>
<div style={{flex: '1 1 240px'}}>
<SearchInput
placeholder='Domain filter...'
@ -987,7 +988,13 @@ function ArchiveSettingsPage() {
onClick={() => navigator.clipboard.writeText('url')}
/>;
return <PageContainer>
/*
* A fragment, not a `PageContainer`. `ArchiveRoute` already wraps its `<Routes>` in one, and
* a second applies its `margin-top: 1em` and `padding: 1em` again -- so this page sat 1em
* lower and 1em further in on all four sides than /archives/domains beside it. As a fragment
* these panels become blocks of the route's own page stack, which spaces them.
*/
return <>
<Panel>
<Header as='h3'>Archive Downloader Config</Header>
@ -1081,7 +1088,7 @@ function ArchiveSettingsPage() {
action={urlFieldNameClipboardButton}
/>
</Panel>
</PageContainer>
</>
}
function ArchivesPage() {

5
app/src/components/Channels.js
View file

@ -499,8 +499,9 @@ export function ChannelsPage() {
}
}, {enableOnFormTags: false});
// Header section matching DomainsPage pattern
const header = <Group justify='space-between' align='flex-end' wrap='wrap' mb='1em'>
// Header section matching DomainsPage pattern. 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.
const header = <Group justify='space-between' align='flex-end' wrap='wrap'>
<SearchInput
placeholder='Name filter...'
size='large'

83
app/src/components/Common.test.js
View file

@ -1,4 +1,6 @@
import React from 'react';
import fs from 'fs';
import path from 'path';
import {act, render, screen, waitFor} from '../test-utils';
import userEvent from '@testing-library/user-event';
import {
@ -724,4 +726,85 @@ describe('PageContainer', () => {
expect(stacks).toHaveLength(2);
stacks.forEach(stack => expect(stack.textContent).toBe('Body'));
});
it('is never rendered by a page that a PageContainer already wraps', () => {
/*
* `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
* in on all four sides than every other page in the app. `/archives/settings` did this
* -- `ArchiveRoute` wraps its `<Routes>` in one, and `ArchiveSettingsPage` wrapped itself
* 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.
*/
const src = path.join(__dirname, '..');
const files = (dir) => fs.readdirSync(dir, {withFileTypes: true}).flatMap(entry => {
const full = path.join(dir, entry.name);
if (entry.isDirectory()) return files(full);
if (!/\.jsx?$/.test(entry.name)) return [];
if (/\.(test|cy)\.jsx?$/.test(entry.name)) return [];
return [full];
});
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>')) {
offenders.push(`${path.relative(src, file)}: ${name}`);
}
}
}
}
/*
* 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.
*/
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);
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.
*/
const routeWrapperBlocks = (text) => {
const blocks = [];
let from = 0;
for (;;) {
const start = text.indexOf('<PageContainer>', from);
if (start === -1) return blocks;
const end = text.indexOf('</PageContainer>', start);
if (end === -1) return blocks;
const block = text.slice(start, end);
if (/<Routes>|<Outlet\s*\/>/.test(block)) blocks.push(block);
from = end + 1;
}
};