mirror of
https://github.com/lrnselfreliance/wrolpi.git
synced 2026-10-26 19:01:39 -04:00
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.
This commit is contained in:
Roland
parent
6be7f4b99e
commit
eba9bbf7de
10 changed files with 269 additions and 125 deletions
|
|
@ -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');
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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}) {
|
|||
<Icon name={collapsed ? "chevron right" : "chevron down"} size="small"/>
|
||||
</>}
|
||||
</div>
|
||||
{!collapsed && <div style={{padding: "4px 12px 8px"}}>
|
||||
{!collapsed && <div style={{padding: "0.25rem 0.75rem 0.5rem"}}>
|
||||
{Object.keys(LAYER_GROUPS).map(name =>
|
||||
<div key={name} style={{padding: "2px 0"}}>
|
||||
<div key={name} style={{padding: "0.125rem 0"}}>
|
||||
<Checkbox
|
||||
label={name}
|
||||
checked={visibility[name]}
|
||||
|
|
@ -312,7 +312,7 @@ function LayerControl({map, scaleUnit, onScaleUnitChange, visibilityRef}) {
|
|||
/>
|
||||
</div>
|
||||
)}
|
||||
<div style={{borderTop: "1px solid var(--border)", marginTop: 6, paddingTop: 6}}>
|
||||
<div style={{borderTop: "1px solid var(--border)", marginTop: "0.375rem", paddingTop: "0.375rem"}}>
|
||||
<Toggle
|
||||
label={scaleUnit === "imperial" ? "Imperial" : "Metric"}
|
||||
checked={scaleUnit === "imperial"}
|
||||
|
|
@ -334,31 +334,31 @@ function AddPinDialog({lat, lon, onSubmit, onCancel}) {
|
|||
return <div style={{
|
||||
position: "absolute", top: "50%", left: "50%", transform: "translate(-50%, -50%)",
|
||||
zIndex: 1002, background: "var(--panel)", border: "1px solid var(--border)", color: "var(--text)",
|
||||
padding: 16, minWidth: 250,
|
||||
padding: "1rem", minWidth: "15.625rem",
|
||||
}}>
|
||||
<div style={{fontWeight: 600, marginBottom: 8}}>Add Pin</div>
|
||||
<div style={{fontSize: '0.75rem', color: "var(--muted)", marginBottom: 8}}>{lat.toFixed(4)}, {lon.toFixed(4)}</div>
|
||||
<div style={{fontWeight: 600, marginBottom: "0.5rem"}}>Add Pin</div>
|
||||
<div style={{fontSize: '0.75rem', color: "var(--muted)", marginBottom: "0.5rem"}}>{lat.toFixed(4)}, {lon.toFixed(4)}</div>
|
||||
<TextInput
|
||||
placeholder="Label"
|
||||
value={label}
|
||||
onChange={e => setLabel(e.target.value)}
|
||||
autoFocus
|
||||
onKeyDown={e => e.key === "Enter" && label.trim() && onSubmit(label.trim(), color)}
|
||||
style={{marginBottom: 8}}
|
||||
style={{marginBottom: "0.5rem"}}
|
||||
/>
|
||||
<div style={{display: "flex", gap: 6, marginBottom: 12}}>
|
||||
<div style={{display: "flex", gap: "0.375rem", marginBottom: "0.75rem"}}>
|
||||
{PIN_COLORS.map(c =>
|
||||
<div
|
||||
key={c}
|
||||
onClick={() => 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)",
|
||||
}}
|
||||
/>
|
||||
)}
|
||||
</div>
|
||||
<div style={{display: "flex", gap: 8, justifyContent: "flex-end"}}>
|
||||
<div style={{display: "flex", gap: "0.5rem", justifyContent: "flex-end"}}>
|
||||
<Button role='cancel' size='sm' onClick={onCancel}>Cancel</Button>
|
||||
<Button
|
||||
role='primary'
|
||||
|
|
@ -484,8 +484,8 @@ function MapSearch({map}) {
|
|||
// Floating search chrome over the canvas — page interface, not map data, so it is
|
||||
// never filtered. Flat surfaces: bordered, no box-shadow, token colors only.
|
||||
return <div ref={containerRef} style={{
|
||||
position: "absolute", top: 10, left: "50%", transform: "translateX(-50%)",
|
||||
zIndex: 5, width: 320, maxWidth: "calc(100% - 100px)",
|
||||
position: "absolute", top: "0.625rem", left: "50%", transform: "translateX(-50%)",
|
||||
zIndex: 5, width: "20rem", maxWidth: "calc(100% - 100px)",
|
||||
}}>
|
||||
<input
|
||||
type="text"
|
||||
|
|
@ -495,20 +495,20 @@ function MapSearch({map}) {
|
|||
onFocus={() => results.length > 0 && setShowResults(true)}
|
||||
onKeyDown={handleKeyDown}
|
||||
style={{
|
||||
width: "100%", padding: "8px 12px", border: "1px solid var(--border)",
|
||||
width: "100%", padding: "0.5rem 0.75rem", border: "1px solid var(--border)",
|
||||
fontSize: '0.875rem', background: "var(--panel)", color: "var(--text)",
|
||||
}}
|
||||
/>
|
||||
{showResults && results.length > 0 && <div style={{
|
||||
background: "var(--panel)", border: "1px solid var(--border)", borderTop: "none",
|
||||
maxHeight: 300, overflowY: "auto",
|
||||
maxHeight: "18.75rem", overflowY: "auto",
|
||||
}}>
|
||||
{results.map((r, i) => <div
|
||||
key={i}
|
||||
onClick={() => handleSelect(r)}
|
||||
onMouseEnter={e => e.currentTarget.style.background = "var(--head)"}
|
||||
onMouseLeave={e => e.currentTarget.style.background = "transparent"}
|
||||
style={{padding: "8px 12px", cursor: "pointer", borderBottom: "1px solid var(--border)"}}
|
||||
style={{padding: "0.5rem 0.75rem", cursor: "pointer", borderBottom: "1px solid var(--border)"}}
|
||||
>
|
||||
<div style={{fontWeight: 500}}>{r.name}</div>
|
||||
{resultSubtext(r) && <div style={{fontSize: '0.6875rem', color: "var(--muted)"}}>{resultSubtext(r)}</div>}
|
||||
|
|
@ -933,14 +933,14 @@ export default function MapViewer() {
|
|||
top: contextMenu.y,
|
||||
zIndex: 1001,
|
||||
background: "white",
|
||||
borderRadius: 6,
|
||||
borderRadius: "0.375rem",
|
||||
boxShadow: "0 2px 8px rgba(0,0,0,0.3)",
|
||||
padding: "4px 0",
|
||||
padding: "0.25rem 0",
|
||||
fontSize: '0.8125rem',
|
||||
minWidth: 180,
|
||||
minWidth: "11.25rem",
|
||||
}}
|
||||
>
|
||||
<div style={{padding: "6px 12px", color: "#666", fontSize: '0.75rem', borderBottom: "1px solid #eee"}}>
|
||||
<div style={{padding: "0.375rem 0.75rem", color: "#666", fontSize: '0.75rem', borderBottom: "1px solid #eee"}}>
|
||||
{(() => {
|
||||
const map = mapRef.current;
|
||||
const zoom = map ? map.getZoom() : 10;
|
||||
|
|
@ -949,7 +949,7 @@ export default function MapViewer() {
|
|||
})()}
|
||||
</div>
|
||||
<div
|
||||
style={{padding: "8px 12px", cursor: "pointer", userSelect: "none"}}
|
||||
style={{padding: "0.5rem 0.75rem", cursor: "pointer", userSelect: "none"}}
|
||||
onMouseEnter={e => e.target.style.background = "#f0f0f0"}
|
||||
onMouseLeave={e => e.target.style.background = "transparent"}
|
||||
onClick={handleCopyCoords}
|
||||
|
|
@ -957,7 +957,7 @@ export default function MapViewer() {
|
|||
Copy GPS coordinates
|
||||
</div>
|
||||
<div
|
||||
style={{padding: "8px 12px", cursor: "pointer", userSelect: "none"}}
|
||||
style={{padding: "0.5rem 0.75rem", cursor: "pointer", userSelect: "none"}}
|
||||
onMouseEnter={e => e.target.style.background = "#f0f0f0"}
|
||||
onMouseLeave={e => e.target.style.background = "transparent"}
|
||||
onClick={handleExportPng}
|
||||
|
|
@ -965,7 +965,7 @@ export default function MapViewer() {
|
|||
Export as PNG
|
||||
</div>
|
||||
<div
|
||||
style={{padding: "8px 12px", cursor: "pointer", userSelect: "none"}}
|
||||
style={{padding: "0.5rem 0.75rem", cursor: "pointer", userSelect: "none"}}
|
||||
onMouseEnter={e => e.target.style.background = "#f0f0f0"}
|
||||
onMouseLeave={e => e.target.style.background = "transparent"}
|
||||
onClick={handleAddPin}
|
||||
|
|
@ -973,7 +973,7 @@ export default function MapViewer() {
|
|||
Add Pin Here
|
||||
</div>
|
||||
<div
|
||||
style={{padding: "8px 12px", cursor: "pointer", userSelect: "none"}}
|
||||
style={{padding: "0.5rem 0.75rem", cursor: "pointer", userSelect: "none"}}
|
||||
onMouseEnter={e => e.target.style.background = "#f0f0f0"}
|
||||
onMouseLeave={e => e.target.style.background = "transparent"}
|
||||
onClick={handleSetDefaultLocation}
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
import React from 'react';
|
||||
import {Loader as MLoader, Skeleton} from '@mantine/core';
|
||||
import {Icon} from './Icon';
|
||||
import {pxToRem} from './scale';
|
||||
import {RoleName, SemanticColorName} from '../../themes/mantine';
|
||||
|
||||
/*
|
||||
|
|
@ -224,7 +225,7 @@ export function Placeholder({lines = 3, height = 12}: {lines?: number; height?:
|
|||
{Array.from({length: lines}, (_, index) => <Skeleton
|
||||
key={index}
|
||||
// In rem so a placeholder line matches the height of the text it stands in for.
|
||||
height={`${height / 16}rem`}
|
||||
height={pxToRem(height)}
|
||||
// A ragged last line reads as text rather than a block.
|
||||
width={index === lines - 1 ? '60%' : '100%'}
|
||||
radius={0}
|
||||
|
|
|
|||
|
|
@ -1,6 +1,7 @@
|
|||
import React from 'react';
|
||||
import {Card as MCard, Tabs as MTabs, Accordion as MAccordion, Breadcrumbs} from '@mantine/core';
|
||||
import {Icon} from './Icon';
|
||||
import {pxToRem} from './scale';
|
||||
|
||||
/*
|
||||
* Surfaces and structure: headers, panels, cards, statistics, tabs, accordions,
|
||||
|
|
@ -225,7 +226,7 @@ export function CardGroup({children, minWidth = 200, className, style, ...props}
|
|||
* The prop stays a px number: seven call sites pass one, and a caller thinking in
|
||||
* px is thinking about the unscaled design.
|
||||
*/
|
||||
gridTemplateColumns: `repeat(auto-fill, minmax(${minWidth / 16}rem, 1fr))`,
|
||||
gridTemplateColumns: `repeat(auto-fill, minmax(${pxToRem(minWidth)}, 1fr))`,
|
||||
...style,
|
||||
}}
|
||||
{...props}
|
||||
|
|
|
|||
|
|
@ -11,6 +11,7 @@
|
|||
|
||||
import './ui.css';
|
||||
|
||||
export * from './scale';
|
||||
export * from './Icon';
|
||||
export * from './Button';
|
||||
export * from './Feedback';
|
||||
|
|
|
|||
20
app/src/components/ui/scale.ts
Normal file
20
app/src/components/ui/scale.ts
Normal file
|
|
@ -0,0 +1,20 @@
|
|||
/*
|
||||
* The interface scale, for the few places that must express a length in JavaScript.
|
||||
*
|
||||
* Sizes belong in ui.css, in rem, where `--ui-scale` reaches them (see themes/tokens.css).
|
||||
* Two components cannot do that, because the length is a prop rather than a design decision:
|
||||
* CardGroup's `minWidth` sets a grid track and Placeholder's `height` sets a skeleton line.
|
||||
* Both took a px number from the caller and both divided it by 16 inline, which put the
|
||||
* design-time base in two places with nothing tying them together.
|
||||
*/
|
||||
|
||||
/** The px-per-rem the design is written against, and the browser's own default. */
|
||||
export const REM_BASE = 16;
|
||||
|
||||
/**
|
||||
* A px length from a call site, as a rem string, so it grows with the interface scale.
|
||||
*
|
||||
* Callers keep thinking in px deliberately: a number in a prop is a statement about the
|
||||
* unscaled design, which is the size the value was chosen at.
|
||||
*/
|
||||
export const pxToRem = (px: number): string => `${px / REM_BASE}rem`;
|
||||
|
|
@ -30,6 +30,92 @@ const uiScale = () => {
|
|||
return value;
|
||||
};
|
||||
|
||||
describe('the interface scale actually reaches what it is supposed to', () => {
|
||||
/*
|
||||
* Every other check on the scale reads source text, which proves a declaration says `rem`
|
||||
* and nothing about whether the browser then paints a larger control. This is the one
|
||||
* that measures, and it is the reason the source guards are worth trusting: it fails if
|
||||
* `--ui-scale` stops being applied to the root, if Mantine's `theme.scale` is set and
|
||||
* cancels it, or if a rule someone adds later pins a px size over the top.
|
||||
*/
|
||||
/*
|
||||
* The root of the application-under-test, not the runner's. Cypress mounts the
|
||||
* component in an iframe and loads the stylesheets there, so `window.top` reads the
|
||||
* runner's own document -- where `--ui-scale` is not declared, so it came back NaN.
|
||||
* `Cypress.$` is scoped to the AUT.
|
||||
*/
|
||||
const root = () => Cypress.$('html')[0];
|
||||
|
||||
it('makes the root font-size the declared multiple of the browser default', () => {
|
||||
cy.mountUI(<Button>Save</Button>);
|
||||
|
||||
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(<Card title='Rainwater Harvesting' meta='engineering775.com'/>);
|
||||
|
||||
/*
|
||||
* 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(<Button>Save</Button>);
|
||||
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(<Panel>Structure</Panel>);
|
||||
|
||||
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('#', '');
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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(/(?<![\w.-])(\d+(?:\.\d+)?)px/g)]
|
||||
.filter(match => 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(/(?<![\w.])(-?\d+(?:\.\d+)?)px/g)]
|
||||
// 0 has no unit to scale, and 1px either way is a hairline.
|
||||
.filter(match => 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([]);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue