chore(react): impeccable audit — drift hex, loader, side-stripe, edit feedback, icon-family consolidation, polish - #136
Draft
romer8 wants to merge 5 commits into
Conversation
…-during-streaming feedback, polish
Resolves three P0s and two P1s from the 2026-05-29 impeccable critique
(`.impeccable/critique/2026-05-29T14-25-24Z__reactapp.md`).
PRODUCT.md + DESIGN.md ship for the first time:
- PRODUCT.md: register=product, primary user is the water-resource scientist,
brand personality "modern, fast, confident" (Linear/Figma/Stripe lane),
four named anti-references (SaaS slop, govt-portal 2010s, crypto neon,
consumer fitness warmth), five strategic design principles.
- DESIGN.md (Stitch format): "The Forecasting Desk" north star, Restrained
color strategy with Watershed Slate (#1e6b8b) as ink not fill, Inter +
JetBrains Mono type stack, 4px structural radius, flat-by-default
elevation, named rules (One Voice, Trace, Drift Kill, Two Families,
Title Anchor, No Rest Shadow, Single Step), full Do's and Don'ts.
- .impeccable/design.json sidecar with token metadata, tonal ramps,
shadow / motion / breakpoint tokens, 7 component HTML/CSS snippets.
Step 1 (harden, drift hex codemod):
- Delete `html { --bs-primary: #0a62a9; }` override in LoadingAnimation.scss
(Drift Kill rule violated on the brand-truth file itself).
- Codemod the three drift values (#007bff, #0d6efd, #0a62a9) to
var(--bs-primary) across 10 sites: DashboardItem.js (inline + react-icons),
DashboardTabs.js (focus outline), DashboardCard.js (focus border),
CustomPicker.js (selected border), Slider.js (rc-slider handle/track),
VisualizationSelector.js (button background), RampPicker.js (selected
state), PreviewCanvas.js (3 sites), map/utilities.js (SVG marker fill).
Step 2 (quieter, planetary loader):
- Replace 24-element planetary loader (2 orbits × 4 arcs + 2 moons + 40pt
label, ~160 lines of SCSS) with a single 32px CSS spinner + body-size
label, gated on prefers-reduced-motion: reduce. Same `(delay, text)`
component API so the 4 call sites Just Work. setTimeout cleanup
included (the prior LoadingAnimation.js:13 leak fixed too).
- Tests updated to match new DOM (.loader / .loader__spinner /
.loader__label) + new aria-live="polite" status region assertion.
Step 3 (distill, side-stripe family):
- SourcePane.js:107 GeoTIFFHint: drop `border-left: 3px solid #0d6efd`,
swap `background: #e7f1ff` to a primary-tinted neutral, swap dark blue
text to ink. Now a flat token card.
- Map.js:345 OpenLayers popup spinner: drop #3498db (a fourth drift blue)
+ #f3f3f3 (Trace Rule), use primary-tinted alpha + canonical primary;
add prefers-reduced-motion guard.
- DashboardTabs.js:103 active tab: drop `border-left/right: 1px solid
#999`, use `box-shadow: inset 0 -2px 0 var(--bs-primary)` bottom
indicator. Govt-portal pattern gone.
Step 4 (clarify, edit-during-streaming silent no-op):
- DashboardItem.js: the three `if (isStreaming) return;` guards (delete,
edit, reorder) now dispatch a CustomAlert via the existing warning
surface ("Editing paused while the chatbox is updating this dashboard.")
before returning. Heuristic #1 fix (Visibility of System Status);
reuses the existing alert system at :540-551, zero new UI surface.
Step 6 (polish):
- DashboardItem.js: AttributionTooltip swaps rgba(0,0,0,0.97) + #ffffffff
to rgba(21,32,42,0.97) + #fbfcfc. Trace Rule.
- ChatSidebar.js: Wrapper drops `transition: width, min-width` (banned
layout-property animation); background #fff → #fbfcfc (Trace Rule);
PatchRejectedBanner govt-portal yellow palette (#fff8e1 / #f0d68a /
#5a4400) → DESIGN.md warn token (#c25a14 tinted); welcome subtitle
em-dash + "just ask" fitness-warm purged to "Create visualizations,
edit tiles, add map layers, analyze data."
- LayersControl.js: ProgressFill animation changed from `transition:
width` (banned) to transform: scaleX() with transform-origin: left;
background swapped from #3498db (fourth drift blue) to
var(--bs-primary).
Step 5 (icon-family codemod, 12 → 1) explicitly DEFERRED. ~50+ files,
name-mapping risks (not every Fa icon has a Bs equivalent), needs visual
smoke. Should be its own PR with codemod + ESLint rule + manual smoke.
Tests: 905 passed, 1 skipped, 0 failed across all touched dirs.
React bundle rebuilt: main.2314ae540dbc52308c47.js → main.1fe08b78e4c886c72cc6.js.
Trend for `reactapp` (last 2 critique runs): 16 → 23. The H+Q+D sweep
should push this further on the next critique pass.
Step 5 of the 2026-05-29 impeccable audit. Closes the most visible
"different person built each panel" signal called out in the critique:
twelve icon families (Bs/Fa/Fa6/Bi/Cg/Ci/Hi/Lu/Md/Ri/Rx/Tb) were mixed
across reactapp/components/, with Bs (rounded) and Fa (sharper) sitting
next to each other on every dashboard card header. DESIGN.md "Two
Families Rule" (typography) extended in spirit to icons: one library.
Codemod approach: alias imports. Each non-Bs identifier is replaced by
`import { BsEquivalent as OldName } from "react-icons/bs"` so usage
sites need zero changes. Files touched (17):
- inputs/Slider.js: 6 FaPlay/Stop/FastForward/Forward/FastBackward/
Backward → BsPlayFill/StopFill/FastForwardFill/SkipForwardFill/
RewindFill/SkipBackwardFill.
- inputs/TextEditor.js: 20 Lu* (tiptap toolbar) → Bs* equivalents
(BsTypeBold, BsTypeItalic, BsTextLeft, BsListUl, BsTextIndentLeft, etc.).
- modals/DataViewer/BorderSettings.js: 5 CgBorder* → BsBorder*.
- map/DrawInteractions.js: TbPointFilled/MdHorizontalRule/FaDrawPolygon/
BiRectangle → BsCircleFill/BsDash/BsBoundingBoxCircles/BsSquare.
- map/LayersControl.js: 4 Fa* → Bs* (BsStack/BsX/BsExclamationTriangle/
BsArrowClockwise).
- map/LegendRenderer.js: RiRectangleFill/RiAddFill → BsSquareFill/BsPlus.
- map/LegendControl.js, visualizations/Map.js, modals/PopupModal/
PopupModal.js: FaTimes → BsX.
- layout/Header.js: HiUserGroup/CiUndo/FaPlus(fa6)/FaExpandArrowsAlt/
FaLock/FaUnlock → BsPeople/BsArrowCounterclockwise/BsPlus/
BsArrowsFullscreen/BsLockFill/BsUnlockFill.
- landingPage/DashboardCard.js: FaPlus/FaRegUserCircle → BsPlus/
BsPersonCircle.
- inputs/DatePicker.js: FaRegCalendarAlt → BsCalendar.
- appTour/AppTour.js, inputs/custom/AddMapLayer.js, modals/MapLayer/
LegendPane.js, modals/MapLayer/PopupLayoutEditor.js: single-import
swaps.
ESLint guardrail added to package.json eslintConfig: no-restricted-
imports rule rejects all eleven non-Bs react-icons subpaths with a
message pointing at the Two Families Rule. Future imports from
react-icons/fa, /fa6, /bi, /cg, /ci, /hi, /lu, /md, /ri, /rx, /tb fail
at lint time.
Visual smoke: the alias-import pattern means JSX usage sites are
unchanged — the same identifier renders a Bootstrap Icon glyph instead
of the prior family's glyph. Closest-match table:
- FaPlus/FaPlus(fa6)/RiAddFill → BsPlus (same "+")
- FaTimes → BsX (same "×")
- FaRegUserCircle → BsPersonCircle (same person-in-circle)
- HiUserGroup → BsPeople (same multi-person)
- LuBold/Italic/Underline/Strikethrough → BsTypeBold/Italic/Underline/
Strikethrough (Bootstrap's Aa-prefix glyphs)
- CgBorderLeft/Top/Right/Bottom/All → BsBorderLeft/Top/Right/Bottom/All
- FaDrawPolygon → BsBoundingBoxCircles (closest "draw polygon" semantic)
- BiRectangle → BsSquare (Bs has no "rectangle" outline; square is the
closest single-rect glyph)
- LuBaseline → BsType (no exact "baseline" match; generic "type" Aa)
Tests: full reactapp test suite — 1853 passed, 4 skipped, 2 pre-existing
failures (DatePicker timezone offset + ModuleLoader loadESRIJSON,
verified against parent commit 48dd55c which fails identically).
Zero new failures from the codemod.
React bundle: 10.1 MiB → 9.33 MiB. Webpack tree-shakes the eleven
removed icon family submodules; ~800 KiB win.
…t, EmptyState, DataTable typeset, polish
Resolves all 2 P0s and both P1s from the 2026-05-29 visualizations
critique (`.impeccable/critique/2026-05-29T15-16-44Z__reactapp-components-visualizations.md`).
Prior critique scored the viz renderers 21/40 — MORE fractured than the
chrome (which scored 23/40 before the prior PR fixed it) because the
chrome critique cleaned chrome but never touched the tile renderers.
Step 1 (distill, plotlyTheme.js + BasePlot.js):
- New plotlyTheme.js exports FORECASTING_DESK_LAYOUT_DEFAULTS,
FORECASTING_DESK_CONFIG_DEFAULTS, FORECASTING_DESK_TOKEN_COLORS, and
a deepMerge helper. Layout sets paper/plot bg to tinted paper,
colorway to Watershed Slate + 2 neutral derivatives (single-accent,
no rainbow), Inter font, tinted hover labels, Rule borders, transparent
legend. Config disables Plotly logo + modebar default-on + the 4
always-confusing buttons.
- BasePlot.js: imports the theme, deep-merges plugin-supplied layout
OVER defaults (plugin authors can extend but not break), wraps
setPlotLayout in deepMerge, wraps Plot config prop in deepMerge.
- createVerticalLine default color: "red" → FORECASTING_DESK_TOKEN_COLORS.primary
(the canonical Watershed Slate). Red was misappropriating the danger
semantic for a generic vertical reference.
Step 2 (quieter, Card.js):
- Complete rewrite from the SaaS hero-metric template (colored icon
chip + 1.5rem bold value + 0.9rem gray label, border-radius 10px) to
the Forecasting Desk value tile (12px leading dot for plugin-supplied
color, 12px monochrome inline icon for plugin-supplied icon, value
rendered in JetBrains Mono tabular-nums, baseline-aligned across the
row via grid-template-columns auto-fit). Plugin contract
{color, label, value, icon} preserved — backward-compatible at the
data layer.
- Fixed Card.js:75 key collision and :80 label-defaults-to-0 bug along
the way.
- Empty data → "No data." in EmptyShell, not "No Data found" inside a
value cell.
Step 3 (clarify, EmptyState.js + 4 renderer migrations):
- New EmptyState.js: title + hint + details (collapsed behind
<details>) + onRetry. Variants: empty (striped paper), error
(warn-tinted), info (sunken paper). Extracted from Base.js's
FeaturePendingShell pattern.
- Image.js: <h2>{imageError ?? "Failed to get image."}</h2> →
<EmptyState variant="error" title="Image unavailable" hint={...}/>
- ImageCollection.js: <h6>...<#888 ErrorText> → same EmptyState.
Dropped ErrorText styled-component entirely.
- ModuleLoader.js: <h2>No system specified</h2> + <h2>Failed to load
remote: {props.url}</h2> → EmptyState variants. URL hidden behind
<details> instead of leaking into a heading.
- ModuleLoader.js: console.log("[ModuleLoader] props:", props) removed
(was shipping to production). Conditional-hook eslint-disable
resolved by unconditionally calling useDynamicFederatedComponent
before the early-return branch.
- Base.js: vizError <StyledH2>{vizData.error}</StyledH2> → EmptyState
variant="error".
Step 4 (typeset, DataTable.js):
- Complete rewrite from <Table striped bordered hover> + <h2> title +
<th> in body rows. New: bare <table>, sticky <thead>, subtle
row-bottom borders only, no zebra/grid/hover-stripe noise.
- detectNumericColumns: a column is numeric iff every non-null cell is
a finite Number. Numeric columns get font-variant-numeric:tabular-nums
+ JetBrains Mono + right-align. Mixed columns stay proportional and
left-aligned.
- Header text rendered verbatim. capitalizePhrase dropped — the prior
function was renaming the scientist's own column names (`comid` →
`Comid`).
- Body cells use <td> not <th> (a11y regression fix).
Step 5 (polish):
- Map.js: OverlayContentWrapper box-shadow removed (flat-by-default
rule). border-radius 8px → 4px. background "white" → tinted paper.
border added (1px Rule).
- Map.js: spinner element creation moved from render path to useMemo
(was constructed via document.createElement on every render,
duplicating the <style> tag globally). Animation name renamed to
ts-map-spin to avoid colliding with the new LoadingAnimation.scss
keyframes.
- LiveChat.js: ChatBubble box-shadow removed.
- VariableInput.js:342: refresh button variant="warning" (orange) →
"outline-secondary". Orange was wrong semantic; reserved for
destructive/attention.
- Base.js: StyledSpinner variant="info" → "primary" at the two
spinner sites. The "info" platform-light-blue was a different visual
language than the new LoadingAnimation; primary aligns with
custom-bootstrap.scss $primary canonical Watershed Slate.
Tests: full reactapp/__tests__/components/ suite passes minus the 2
pre-existing failures (ModuleLoader loadESRIJSON, DatePicker timezone
— both verified identical to parent PR #136 baseline). Zero new
regressions. Visualizations sub-suite: 283/283 pass after test
assertion updates for the new copy.
Test assertion updates (the prior copy now reads differently):
- BasePlot.test.js: vertical-line color "red" → "#1e6b8b" (7 sites).
- Image.test.js: "Failed to get image." → "Image unavailable".
- imageCollection.test.js: same + getAllByText handling for the
title+hint dual-text case + a custom-error-test prop value change
to disambiguate from the title.
- DataTable.test.js: "No Data Available" → "No data." (2 sites). Column
header assertions for "Name"/"Age"/"Occupation" → "name"/"age"/
"occupation" (capitalizePhrase dropped; RTL whitespace-normalizes).
- ModuleLoader.test.js: "No system specified" → "No module specified"
(5 sites). "Failed to load remote: <URL>" → "Could not load module".
React bundle rebuilt: main.02c988dc0a7dd633243f.js → main.d5d76bea18f45d0fdffa.js.
Bundle size unchanged (still 9.33 MiB) — new plotlyTheme + EmptyState
land in the same chunk as the existing viz code.
Critique trend for reactapp-components-visualizations: 21 (first run).
Re-run after merge to measure the post-fix delta; projected landing
zone after this PR: 30-34.
LiveChat.js Material Design 2018 palette refactor (P2) deferred — own
PR with usage investigation first.
Follow-up to the 2026-05-29 viz audit (commit ec569cc). The map popup ships its own table separate from <DataTable/> — so it never picked up the bare-table + sticky-thead + tabular-nums + verbatim-headers typeset pass. Visible in a Brave screenshot of a CONUS flowpaths popup that still rendered as react-bootstrap striped/bordered/hover. Map.js: - Replace <FixedTable striped bordered hover size="sm"> (react-bootstrap <Table>) with bare <table> styled-component (PopupTable) that mirrors DataTable.js: subtle row-bottom borders only, paper-raised thead, uppercase label-style column headers, tabular-nums + JetBrains Mono + right-align for numeric column cells (autodetected per cell since the popup is column-of-key + column-of-mixed-value, not a typed table). - Drop react-bootstrap <Table> import entirely. - Replace the ❮ / ❯ Unicode swiper arrows (font-size: 24px; color: #333) with BsChevronLeft / BsChevronRight icons inside <button> elements, styled to match DESIGN.md (4px radius, Watershed Slate hover, ring focus state). - Swiper pagination switched to label-type (11px / 500 / .04em tracking / Ink Muted). - aria-label "Previous Swiper" / "Next Swiper" → "Previous feature" / "Next feature" — more accurate description of what the control does. - PopupHeading replaces <p><b>{layerName}</b>:</p> with a title-typed heading; layerName trails without the trailing colon. Test updates: Map.test.js:1578 aria-label assertion swapped from "Next Swiper" to "Next feature" to match new copy. 61/61 Map tests pass. React bundle rebuilt: main.d5d76bea18f45d0fdffa.js → main.336bac67929c21da156d.js.
Follow-up to the 2026-05-29 critique after a Brave screenshot of the landing page surfaced two named DESIGN.md violations the critique hadn't called out specifically: 1. The header navbar shipped bg="primary" variant="dark" — fills the entire top region with Watershed Slate. DESIGN.md Section 5 Components > Header explicitly says Paper background NOT Watershed Slate fill (Committed strategy, which we rejected). DESIGN.md §2 "The One Voice Rule": accent ≤10% as ink not fill. 2. The landing page rendered 12 dashboard cards as identical icon+heading+text repeats with the placeholder chart-icon thumbnail on every card. DESIGN.md absolute ban "Identical card grids." PRODUCT.md anti-reference #1. layout/Header.js: - Both <CustomNavBar fixed="top" bg="primary" variant="dark" className="shadow"> sites (LandingPageHeader :164 + DashboardHeader :475) → bg="light" variant="light". Drop className="shadow" (flat-by-default). - CustomNavBar: add 1px Rule bottom border for the structural anchor the prior shadow was providing. - WhiteTitle → Title: rename + restyle from color: white to Ink (#15202a), 18px / 600, baseline-anchored to header height. Trace Rule fix. buttons/HeaderButton.css: - .navbar .btn background rgba(255,255,255,0.1) → transparent, color white → Ink Muted (#4b5b6a). - :hover/:focus: rgba(0,0,0,0.1) → Paper Raised (#f4f6f7), color white → Ink. Buttons now have legible contrast against the new Paper navbar. landingPage/DashboardCard.js: - CustomCard: bg rgb(238,238,238) → Paper (#fbfcfc). Border 1px Rule at rest, Rule Strong on hover (was hover-darken-fill). 4px radius per DESIGN.md scale (was Bootstrap default). min-height 9rem (was fixed 15rem) — variable height kills the identical-grid uniformity. - CardBody: drop hover dark-grey background (was rgba(169,169,169,0.5) flat-by-default violation). - CardHeader: transparent background + 1px Rule bottom border, 8px padding (was Bootstrap-default 12px+). - CardTitle: left-aligned 15px / 600 Ink with ellipsis (was centered h5 with no overflow handling). - CardTitleDiv: flex:1 left-aligned (was centered). - New Monogram styled-component: large first-letter of title rendered bottom-right in JetBrains Mono, 56px / 600, slate-tinted at 16% alpha. The quiet visual differentiator per card. - DescriptionDiv: promoted from hover overlay to in-flow text with 4-line clamp. Cards now differentiate by actual content the editor wrote rather than by hover state. - CardImage styled-component deleted entirely. The illustrative chart-icon placeholder thumbnail is gone. `image` prop kept on the component signature for backward compat with the parent dashboards list + the thumbnail-modal save path; `setDashboardImage` is still called when the modal saves so the API contract is intact. Custom thumbnails can come back as a leading visual mark in a later iteration. Tests: 137/137 pass across landing + header + map suites. - DashboardCard.test.js:246 "edit name and no change" — dropped the `getByLabelText("Dashboard Card Image")` + `image.src` assertion (the rendered image element no longer exists). Replaced with queryByLabelText assertion that the element is null + a comment pointing at the critique decision. React bundle rebuilt: main.336bac67929c21da156d.js → main.33264482ad22b79f59bd.js.
romer8
marked this pull request as draft
July 10, 2026 16:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Full /impeccable critique pass on
reactapp/. Ships PRODUCT.md + DESIGN.md (Stitch format) for the first time, then resolves all 3 P0s and both P1s from the 2026-05-29 critique snapshot (.impeccable/critique/2026-05-29T14-25-24Z__reactapp.md).Trend for
reactapp: 16 → 23 → projected 30–34 after this PR.What landed
harden#007bff/#0d6efd/#0a62a9→var(--bs-primary); delete--bs-primaryoverride inLoadingAnimation.scssquieterprefers-reduced-motion: reducegateddistillSourcePane.js:107,Map.js:345,DashboardTabs.js:103— flat cards / inset shadow indicators instead of govt-portal stripesclarifyif (isStreaming) return;silent no-ops inDashboardItem.jsnow dispatch aCustomAlertvia the existing warning surfacedistillBsX as FaTimes, etc.); ESLintno-restricted-importsrule rejects the eleven other react-icons subpaths going forwardpolish#fff/#000→ tinted tokens; banner palette swap tocolors.warn; transition layout-properties → transformContext
PRODUCT.md— register=product, primary user is the water-resource scientist building dashboards for their team, brand personality "modern, fast, confident" (Linear / Figma / Stripe lane), four named anti-references, five strategic design principles.DESIGN.md— "The Forecasting Desk" north star, Restrained color strategy (Watershed Slate#1e6b8bas ink not fill on ≤10% of any screen), Inter + JetBrains Mono type stack, 4px structural radius, flat-by-default elevation, named rules (One Voice, Trace, Drift Kill, Two Families, Title Anchor, No Rest Shadow, Single Step). Includes the Stitch sidecar at.impeccable/design.jsonwith token metadata + tonal ramps + 7 component HTML/CSS snippets.Test plan
loadESRIJSON— both verified to fail identically against parent commit, unrelated to this PR).DashboardItem.js), the new quiet loader on dashboard load, the tab active-state inset indicator, the patch-rejected banner palette swap, the icon glyph substitutions acrossHeader,DashboardCard,Slider,TextEditor,BorderSettings,DrawInteractions. Worth a 15-min click-through before merge.Out of scope / known follow-ups
#333,#666inChatSidebar.jsheaders + close button) — Trace Rule technically applies but not blocking; next polish pass.