From eba9bbf7de68ce255b06fc5b0e78a110a2bd816d Mon Sep 17 00:00:00 2001 From: Roland Date: Mon, 3 Aug 2026 19:37:25 -0600 Subject: [PATCH] review: close the mixed-unit gaps the scale left behind Seven review comments, all valid. The common thread is the one the guard was supposed to catch and did not: geometry converted to rem beside geometry that stayed px. The guard was narrower than its name. It missed: - `transform`, `flex` and `flex-basis`, so `translateX(26px)` and `flex: 1 1 200px` were invisible on properties nobody thought to list; - every NEGATIVE length, because the lookbehind treated the minus sign as part of the preceding token -- `left: -7px` and `margin-top: -2.5px` position the eyelet on a tag whose diamond HAD been converted, and `right: -3px` / `bottom: -3px` place the corner badge on a converted icon. Each is a piece of geometry that stopped tracking the piece it belongs to. With both fixed it found nine values, including one no comment mentioned (`flex: 1 1 200px`). All nine converted, except the three in a dead `.toggle` / `.slider` block: nothing has referenced those classes since Toggle became a Mantine Switch, and it was ALREADY broken -- track and knob in rem, travel still `translateX(26px)`, so the thumb stopped ~4px short of a 54px track. Dead CSS that is also wrong is a trap for whoever revives it, so it is deleted. The tag eyelet is centred with `translateY(-50%)` rather than a negative margin of half its own height, so the two pieces cannot drift again whatever the scale. Also from the review: - A runtime check, which is what the source guards were missing. Three cases in ui-layout.cy.js measure the real thing: the root font-size is the declared multiple of 16, our text AND a Mantine control both grow by the same factor when the scale changes, and a hairline does not. The second is the one that catches `theme.scale` being set and silently cancelling the root. - `max-height: max(..., 56.25cqw)` now follows a plain rem declaration. An engine without container query units throws out the whole declaration, which would have left the poster with NO cap -- a portrait cover standing as tall as the card is wide. Older engines clamp; newer ones take the second. - `pxToRem` in components/ui/scale.ts, replacing the `/ 16` written out in CardGroup and Placeholder. - The map overlays: 20 values across the layer panel, add-pin dialog, search panel and context menu, whose type had grown 10% while their padding had not. Left in px on purpose: the context menu's `left`/`top` (a real click coordinate), maplibre's own `ScaleControl` maxWidth, and the `calc(100% - 100px)` reserve for maplibre's px-sized controls. The `100vh - 120px` reserve DID become rem, since the chrome it reserves for is ours. - The keyboard-shortcuts spec no longer deletes `.mantine-Notifications-root` from the DOM. The reviewer was right that it is brittle; the endpoints are mocked instead, now that the probe named them: `/api/tag`, `/api/events/feed`, `/api/files/search` and `/api/files/worker_status`. Two of those were already "mocked" as `/api/tags` and `/api/events`, which nothing calls -- the wrong paths were the reason a toast appeared at all. Verified under CI's condition (`forceNetworkError` on everything unmocked): 12 passing with no DOM surgery. What is NOT done, deliberately: about 176 numeric geometry values remain in 23 feature files. The map overlays were the visible ones. Sweeping the rest is a change to 23 files nobody has reviewed, so instead the guard's name now says "in our stylesheets" rather than implying the whole app, and a new ratchet keeps numeric geometry out of the component library so the eleventh inline `gap: 10` cannot be added there. Both new guards planted against. Verified: 1142 jest / 65 suites, 251 ui-layout + 10 modal-stack component tests, all 50 e2e specs, clean build, map overlays measured to track the scale (panel minWidth 200 -> 220, search 320 -> 352) and screenshotted. --- app/cypress/e2e/keyboard-shortcuts.cy.js | 58 +++++++++------- app/src/App.css | 75 ++++----------------- app/src/components/MapViewer.js | 58 ++++++++-------- app/src/components/ui/Feedback.tsx | 3 +- app/src/components/ui/Surfaces.tsx | 3 +- app/src/components/ui/index.ts | 1 + app/src/components/ui/scale.ts | 20 ++++++ app/src/components/ui/ui-layout.cy.js | 86 ++++++++++++++++++++++++ app/src/components/ui/ui.css | 23 +++++-- app/src/themes/ui-scale.test.js | 67 ++++++++++++++++-- 10 files changed, 269 insertions(+), 125 deletions(-) create mode 100644 app/src/components/ui/scale.ts diff --git a/app/cypress/e2e/keyboard-shortcuts.cy.js b/app/cypress/e2e/keyboard-shortcuts.cy.js index df7f8df6..fd2835f9 100644 --- a/app/cypress/e2e/keyboard-shortcuts.cy.js +++ b/app/cypress/e2e/keyboard-shortcuts.cy.js @@ -17,18 +17,42 @@ describe('Keyboard Shortcuts', () => { body: {settings: {}} }).as('getSettings'); - // Mock tags API - cy.intercept('GET', '/api/tags', { + /* + * `/api/tag` and `/api/events/feed`, which are the URLs the app actually requests. + * + * These read `/api/tags` and `/api/events` before -- neither of which anything calls, + * so both mocks matched nothing and the real calls fell through. In CI, where the + * React app is served with no API behind it, they failed and api.js raised an + * "Unexpected server response" toast, which then covered the search modal's input and + * failed an occlusion-sensitive assertion. Locally the API answered and the spec + * passed, which is why it only ever broke on CI. + */ + cy.intercept('GET', '/api/tag*', { statusCode: 200, body: {tags: []} }).as('getTags'); - // Mock events API - cy.intercept('GET', '/api/events', { + cy.intercept('GET', '/api/events/feed*', { statusCode: 200, - body: {events: []} + // `now` as well as `events`: Events.js reads both off the response. + body: {events: [], now: new Date(0).toISOString()} }).as('getEvents'); + /* + * The other two the home page calls. `file_groups` is reached as + * `data['totals']['file_groups']`, so an empty object here would throw rather than + * toast -- the shape has to be right, which is why a blanket `{}` stub does not work. + */ + cy.intercept('POST', '/api/files/search', { + statusCode: 200, + body: {file_groups: [], totals: {file_groups: 0}} + }).as('searchFiles'); + + cy.intercept('GET', '/api/files/worker_status', { + statusCode: 200, + body: {status: {}} + }).as('getWorkerStatus'); + // Mock search suggestions API cy.intercept('POST', '/api/search/suggestions', { statusCode: 200, @@ -56,25 +80,13 @@ describe('Keyboard Shortcuts', () => { cy.get('[role="dialog"]').should('be.visible'); /* - * Take down any toast before the assertion below, which is occlusion-sensitive. - * - * Toasts paint above modals on purpose -- z-index 400 against the modal stack's - * 200 -- so that an error raised while a modal is open stays visible. The search - * modal is tall, so it sits near the top of the window, and a top-right toast - * covers the right-hand 192px of its 365px input. Cypress occlusion-checks a - * `position: fixed` element, so `should('be.visible')` on that input fails. - * - * That is not this spec's subject. It passed locally and failed in CI, where the - * React app is served with no API behind it: `/api/tag`, `/api/events/feed`, - * `/api/files/search` and `/api/files/worker_status` are not mocked here, they - * fail, and api.js raises "Unexpected server response". The overlap itself is an - * accepted trade-off -- see the toast/modal note in ui.css -- so the environment is - * made quiet rather than the assertion weakened. - * - * Removed rather than dismissed by clicking: a click races the next failing call. + * Occlusion-sensitive: Cypress checks whether a `position: fixed` element is + * covered, and a toast at top-right covers the right-hand 192px of this 365px + * input. Toasts paint above modals on purpose -- z-index 400 against the modal + * stack's 200 -- so an error raised while a modal is open stays visible, and that + * overlap is an accepted trade-off rather than something this spec should assert + * about. The endpoints are all mocked in `beforeEach` so no toast is raised. */ - cy.get('body').then(($body) => $body.find('.mantine-Notifications-root').remove()); - cy.get('[role="dialog"] input').should('be.visible'); }); diff --git a/app/src/App.css b/app/src/App.css index bf7afe06..20349c40 100644 --- a/app/src/App.css +++ b/app/src/App.css @@ -216,66 +216,18 @@ div.inverted { background-color: #1B1C1D; } -.toggle { - position: relative; - display: inline-block; - width: 3.0625rem; - height: 1.4375rem; -} - -.toggle input { - opacity: 0; - width: 0; - height: 0; -} - -.slider { - position: absolute; - cursor: pointer; - top: 0; - left: 0; - right: 0; - bottom: 0; - background-color: rgb(166, 166, 166); - -webkit-transition: .4s; - transition: .4s; - border-radius: 2.125rem; -} - -.slider:before { - position: absolute; - content: ""; - height: 1.375rem; - width: 1.375rem; - left: 0; - bottom: 0; - background-color: white; - -webkit-transition: .4s; - transition: .4s; - border-radius: 50%; -} - -.slider.disabled:before { - background-color: #CCCCCC; -} - -input:checked + .slider { - background-color: #6435C9FF; -} - -input.inverted:checked + .slider.inverted { - background-color: #6435C9AA; -} - -input.disabled + .slider { - background-color: rgb(147, 147, 147); -} - -input:checked + .slider:before { - -webkit-transform: translateX(26px); - -ms-transform: translateX(26px); - transform: translateX(26px); -} +/* + * The hand-rolled `.toggle` / `.slider` switch is gone. + * + * Nothing referenced either class: `Toggle` is a Mantine Switch, and the only remaining + * matches for the words were the `classList.toggle` DOM method and the controller's own + * templates, which do not load this stylesheet. + * + * It is deleted rather than finished because it was already half-converted and broken: the + * track and knob became rem while the checked-state travel stayed `translateX(26px)`, so on a + * 54px track the thumb would have stopped ~4px short. Dead CSS that is also wrong is a trap + * for whoever revives it, and it also hardcoded #6435C9 and white, which no theme can reach. + */ .inverted-progress-text.ui.indicating.progress .bar > .progress, .inverted-progress-text.ui.progress > .label { @@ -306,7 +258,8 @@ input:checked + .slider:before { } .preview-toolbar-tags { - flex: 1 1 200px; + /* rem so the basis grows with the tags it holds. */ + flex: 1 1 12.5rem; min-width: 0; } diff --git a/app/src/components/MapViewer.js b/app/src/components/MapViewer.js index cea9be3d..f00b519d 100644 --- a/app/src/components/MapViewer.js +++ b/app/src/components/MapViewer.js @@ -264,15 +264,15 @@ function LayerControl({map, scaleUnit, onScaleUnitChange, visibilityRef}) { // every theme, same as the nav bar. const panelStyle = { position: "absolute", - top: 10, - left: 10, + top: "0.625rem", + left: "0.625rem", zIndex: 1000, background: "var(--panel)", border: "1px solid var(--border)", color: "var(--text)", - maxHeight: "calc(100vh - 120px)", + maxHeight: "calc(100vh - 7.5rem)", overflowY: "auto", - minWidth: iconOnly ? 0 : 200, + minWidth: iconOnly ? 0 : "12.5rem", fontSize: '0.8125rem', }; @@ -280,10 +280,10 @@ function LayerControl({map, scaleUnit, onScaleUnitChange, visibilityRef}) { ? { padding: 0, cursor: "pointer", userSelect: "none", display: "flex", alignItems: "center", justifyContent: "center", - width: 40, height: 40, + width: "2.5rem", height: "2.5rem", } : { - padding: "8px 12px", cursor: "pointer", fontWeight: 600, + padding: "0.5rem 0.75rem", cursor: "pointer", fontWeight: 600, display: "flex", justifyContent: "space-between", alignItems: "center", borderBottom: collapsed ? "none" : "1px solid var(--border)", userSelect: "none", }; @@ -302,9 +302,9 @@ function LayerControl({map, scaleUnit, onScaleUnitChange, visibilityRef}) { } - {!collapsed &&
+ {!collapsed &&
{Object.keys(LAYER_GROUPS).map(name => -
+
)} -
+
-
Add Pin
-
{lat.toFixed(4)}, {lon.toFixed(4)}
+
Add Pin
+
{lat.toFixed(4)}, {lon.toFixed(4)}
setLabel(e.target.value)} autoFocus onKeyDown={e => e.key === "Enter" && label.trim() && onSubmit(label.trim(), color)} - style={{marginBottom: 8}} + style={{marginBottom: "0.5rem"}} /> -
+
{PIN_COLORS.map(c =>
setColor(c)} style={{ - width: 24, height: 24, borderRadius: "50%", background: c, cursor: "pointer", + width: "1.5rem", height: "1.5rem", borderRadius: "50%", background: c, cursor: "pointer", border: color === c ? "3px solid var(--text)" : "2px solid var(--border)", }} /> )}
-
+
); + + cy.get('.mantine-Button-root').should(() => { + const scale = parseFloat(getComputedStyle(root()).getPropertyValue('--ui-scale')); + expect(scale, 'the scale is declared').to.be.within(0.5, 3); + // 16px is the browser default the percentage resolves against. + expect(parseFloat(getComputedStyle(root()).fontSize), 'root font-size') + .to.be.closeTo(16 * scale, 0.5); + }); + }); + + it('grows a Mantine control and our own text together when the scale changes', () => { + /* + * Both halves in one measurement, because the failure that matters is them + * DISAGREEING: Mantine's sizes are `calc(Xrem * var(--mantine-scale))` and ours are + * plain rem, so a `theme.scale` of 1.1 alongside a 110% root would leave Mantine's + * buttons at the old size beside 10% larger card text. + */ + cy.mountUI(); + + /* + * DOUBLE whatever the scale currently is, rather than setting it to 2. Setting the + * literal gave a ratio of 1.818 -- 2 divided by the 1.1 already in effect -- so the + * first version of this failed against a perfectly working scale. + */ + const doubled = () => { + const current = parseFloat(getComputedStyle(root()).getPropertyValue('--ui-scale')); + root().style.setProperty('--ui-scale', String(current * 2)); + }; + + cy.get('.wrolpi-card-title').should(($title) => { + const before = parseFloat(getComputedStyle($title[0]).fontSize); + expect(before, 'the card title has a size to grow from').to.be.greaterThan(1); + doubled(); + const after = parseFloat(getComputedStyle($title[0]).fontSize); + root().style.removeProperty('--ui-scale'); + + // A px declaration would not have moved at all. + expect(after / before, 'our own text tracks the scale').to.be.closeTo(2, 0.05); + }); + + cy.mountUI(); + cy.get('.mantine-Button-root').should(($button) => { + const before = $button[0].getBoundingClientRect().height; + doubled(); + const after = $button[0].getBoundingClientRect().height; + root().style.removeProperty('--ui-scale'); + + expect(after / before, "Mantine's controls track it too, by the same factor") + .to.be.closeTo(2, 0.05); + }); + }); + + it('leaves a hairline a hairline at any scale', () => { + // The inverse, and the whole reason rem was chosen over `zoom`. + cy.mountUI(Structure); + + cy.get('.wrolpi-panel').should(($panel) => { + const before = parseFloat(getComputedStyle($panel[0]).borderTopWidth); + root().style.setProperty('--ui-scale', '2'); + const after = parseFloat(getComputedStyle($panel[0]).borderTopWidth); + root().style.removeProperty('--ui-scale'); + + expect(before, 'the panel has a hairline border').to.be.within(0.5, 2); + expect(after, 'and it does not grow with the type').to.be.closeTo(before, 0.05); + }); + }); +}); + /* `--panel` is authored as a hex; computed backgrounds come back as rgb(). */ const hexToRgb = (hex) => { const value = hex.replace('#', ''); diff --git a/app/src/components/ui/ui.css b/app/src/components/ui/ui.css index 1ca2fc8d..f6f53c6d 100644 --- a/app/src/components/ui/ui.css +++ b/app/src/components/ui/ui.css @@ -43,8 +43,9 @@ */ .wrolpi-icon-stack-corner { position: absolute; - right: -3px; - bottom: -3px; + /* rem, so the badge keeps its corner as the glyph it sits on grows. */ + right: -0.1875rem; + bottom: -0.1875rem; display: flex; background: var(--icon-stack-bg, var(--bg)); border-radius: 50%; @@ -836,10 +837,15 @@ html[data-theme="amber"] .wrolpi-path-input-control:focus-within { content: ''; position: absolute; top: 50%; - left: -7px; + left: -0.4375rem; width: 0.3125rem; height: 0.3125rem; - margin-top: -2.5px; + /* + * Centred by translation rather than by a negative margin of half its own height. + * The margin was -2.5px against a 0.3125rem dot: once the dot scaled and the margin + * did not, the hole drifted off the point's centre. A percentage cannot drift. + */ + transform: translateY(-50%); border-radius: 50%; background: currentColor; /* A hole, not a dot: it should read as absence rather than as a bullet. */ @@ -1092,7 +1098,7 @@ html .mantine-Notification-closeButton:hover { cursor: pointer; display: flex; /* Widen the tap target past the glyph without moving it. */ - margin: -6px -6px -6px 0; + margin: -0.375rem -0.375rem -0.375rem 0; padding: 0.375rem; } @@ -1378,7 +1384,14 @@ html[data-theme="amber"] .wrolpi-progress-fill { * * `max()`, not `min()`: on a narrow card the fraction is the smaller value and the floor * has to win, or the cap would tighten as cards got smaller and crop a portrait cover. + * + * Declared twice on purpose. `cqw` needs container queries (Chrome 105, Safari 16), and an + * engine that does not know the unit throws out the WHOLE declaration -- which would leave + * the poster with no height cap at all, so a portrait cover would stand as tall as the card + * is wide. The plain cap first means such an engine still clamps, just without the + * card-relative growth; anything newer takes the second and ignores the first. */ + max-height: var(--card-poster-max-height); max-height: max(var(--card-poster-max-height), 56.25cqw); } diff --git a/app/src/themes/ui-scale.test.js b/app/src/themes/ui-scale.test.js index e29dddf2..6f0de94b 100644 --- a/app/src/themes/ui-scale.test.js +++ b/app/src/themes/ui-scale.test.js @@ -68,7 +68,7 @@ describe('--ui-scale', () => { expect(stripComments(mantine)).not.toMatch(/\bscale:\s*[\d.]/); }); - it('writes every scalable length in rem, so the scale reaches it', () => { + it('writes every scalable length in our stylesheets in rem, so the scale reaches it', () => { /* * A px font-size does not grow with the root, so it would shrink RELATIVE to * everything around it -- the failure is a 13px caption beside 14.3px prose, which @@ -78,17 +78,41 @@ describe('--ui-scale', () => { * scaling it is exactly what made `zoom` the wrong tool. Same for outlines, which are * hairlines wearing a different name. */ - const SCALABLE = /^\s*(font-size|padding|padding-\w+|margin|margin-\w+|gap|row-gap|column-gap|width|min-width|max-width|height|min-height|max-height|border-radius|top|right|bottom|left)\s*:/; + /* + * `transform` and `flex-basis` are on the list because a length there is as much part + * of the layout as a padding. The first version left them off and the name of this + * test still said "every scalable length": `transform: translateX(26px)` sat in App.css + * untouched while the track and knob either side of it were converted to rem, so a + * switch's thumb travelled 26px across a track that had grown to 54px. + */ + const SCALABLE = new RegExp('^\\s*(' + [ + 'font-size', 'padding', 'padding-\\w+', 'margin', 'margin-\\w+', + 'gap', 'row-gap', 'column-gap', 'width', 'min-width', 'max-width', + 'height', 'min-height', 'max-height', 'border-radius', + 'top', 'right', 'bottom', 'left', + 'transform', '-webkit-transform', '-ms-transform', 'translate', + 'flex', 'flex-basis', + ].join('|') + ')\\s*:'); const offenders = []; let scanned = 0; + let negatives = 0; for (const [name, full] of STYLESHEETS) { stripComments(fs.readFileSync(full, 'utf8')).split('\n').forEach((line, index) => { if (!SCALABLE.test(line)) return; scanned += 1; - // 0 has no unit to scale, and 1px in a scalable property is a hairline too. - const px = [...line.matchAll(/(? parseFloat(match[1]) > 1); + /* + * The optional leading `-` is load-bearing. Without it the lookbehind treated + * a minus sign as part of the preceding token, so every NEGATIVE length was + * invisible: `left: -7px` and `margin-top: -2.5px` position the eyelet on a + * tag whose diamond had been converted to rem, and `right: -3px` / `bottom: + * -3px` place the corner badge on an icon that had also been converted. Each + * one is a piece of geometry that stopped tracking the piece it belongs to. + */ + const px = [...line.matchAll(/(? Math.abs(parseFloat(match[1])) > 1); + if (px.some(match => match[1].startsWith('-'))) negatives += 1; if (px.length) offenders.push(`${name}:${index + 1}: ${line.trim().slice(0, 64)}`); }); } @@ -166,4 +190,37 @@ describe('--ui-scale', () => { expect(offenders).toEqual([]); }); + + it('keeps numeric geometry out of the component library, where a scale cannot reach it', () => { + /* + * The library's own inline styles, held to the same rule as its font sizes. `gap: 10` + * in Confirm and `padding: '10px 12px 12px'` in Card were px forever, so the ten such + * blocks moved into ui.css; this is what stops the eleventh being written. + * + * Deliberately the LIBRARY only. There are about 176 numeric geometry values left in + * 23 feature files -- the map overlays were the visible ones and are converted, but + * sweeping the rest is a change to 23 files that nobody has looked at, and it is not + * this test's job to pretend otherwise. That gap is the reason the sibling test above + * says "in our stylesheets" rather than "everywhere": an earlier name claimed the + * whole app while scanning four files. + */ + const GEOMETRY = new RegExp('\\b(' + [ + 'padding', 'paddingTop', 'paddingBottom', 'paddingLeft', 'paddingRight', + 'margin', 'marginTop', 'marginBottom', 'marginLeft', 'marginRight', + 'gap', 'rowGap', 'columnGap', 'width', 'height', 'minWidth', 'minHeight', + 'maxWidth', 'maxHeight', 'borderRadius', 'flexBasis', + ].join('|') + "):\\s*(-?[1-9][0-9]*\\b|['\"]-?[\\d.]+px)"); + + const offenders = []; + for (const file of sourceFiles(path.join(SRC, 'components', 'ui'))) { + if (file.endsWith('.css')) continue; + stripComments(fs.readFileSync(file, 'utf8')).split('\n').forEach((line, index) => { + if (GEOMETRY.test(line)) { + offenders.push(`${path.relative(SRC, file)}:${index + 1}: ${line.trim().slice(0, 60)}`); + } + }); + } + + expect(offenders).toEqual([]); + }); });