Conversation
- Add Analyze/fixtures.ts with dataset, epoch, channel and plot fixtures. - Add AnalyzeOverview, AnalyzeErp, AnalyzeBehavior pure-props components. - Add Analyze.stories.tsx with all required stories rendered inside the real AppShell chrome at location='analyze'. - Fix ClickableHeadDiagramSVG prop spread so onChannelClick is not passed to the underlying <svg> element. - Copy brief into docs/uxr. Refs: docs/uxr/2026-09-24-ws7-design-brief.md, playtest plan §9/§10.2.
…ture - Make AppShell badges/nextArea configurable per story via parameters. - NoData: nextArea=collect, no badges. - BehaviorBeforeCleaning: only Collect badge (4 recordings), nextArea=clean. - EEG results stories: Collect + Clean badges, nextArea=analyze. - Behavior-only workspace: Collect badge only, nextArea=analyze, Clean tab hidden. - ErpResults: selectedChannel TP9 to match fixture SVG title. - Re-screenshot all stories at 1366x768 and 1280x720; no overflow or console errors.
Contributor
Author
Review fixes (added after Main review)
|
…topo shape - Controls rail + results on every tab; plots fit 1366x768 and 1280x720 without scroll. - CompareOverviewPlotsFirst: plots on top, recordings strip below (review comparison). - Topography fixture now mirrors plot_topo (per-sensor ERPs on a head), built from example epochs. - ERP: graph and sensor picker first; 4-step walkthrough drawn as React SVG from EpochArraysMeta-shaped example epochs, averages computed with meanTrace. - Behavior: rail holds recordings, plot options and export; plot computed by the real aggregateDataForPlot from example CSV rows; export success and failure stories. - Scoped .bw-analyze-plot CSS lets PyodidePlotWidget images shrink to their card.
Contributor
Author
|
Round-2 review (Main): scope, measurements and screenshots checked. The ClickableHeadDiagramSVG change is still only the destructure fix. The new CSS is one scoped Integration note, not a design change: the Behavior chart's two blues come from existing code. |
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
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.
Design pass for Workstream 7 (Analyze), following
docs/uxr/2026-09-24-ws7-design-brief.md(copied into this branch) and plan §8.4, §9 and §10.2 "Analyze". This is Storybook only; runtime behavior does not change. Engineering integrates after approval.AnalyzeComponent.tsx,PyodidePlotWidget.tsx,AnalyzeContainer, the epics, the worker andcompute.jsare unchanged. One small shared edit is included:ClickableHeadDiagramSVGnow destructures its props (details below).Round 2 applies the product feedback. The layout is rebuilt around a controls rail. The topography fixture now matches what
plot_toporeally draws. The ERP tab puts the graph first and adds a four-step walkthrough built from epoch arrays. Behavior is plotted by the realaggregateDataForPlot.Layout decision
Every tab uses one pattern: a 300px controls rail on the left and the results on the right. The page does not scroll. Plots shrink to fit their card instead of making the page taller. The shared pieces live in
Analyze/AnalyzeParts.tsx:AnalyzeLayout,RailSection,DatasetChecklist,ResultStatus,CleanRequired,SegmentedandPlotFigure.PlotFigurewraps the unchangedPyodidePlotWidgetin a scoped.bw-analyze-plotclass inapp.global.css. The class makes the imageflex: 1; min-height: 0; object-fit: contain, so the image keeps its aspect ratio, fills the card, and Save as SVG/PNG stays underneath. No shared global class was changed.<select multiple>, so choosing several recordings needs no Cmd/Ctrl.CompareOverviewPlotsFirstputs the plots on top and a compact recordings strip below. One of it andOverviewResultsgets deleted after review.Measurements
Every story was loaded in Chromium at both sizes. The results:
"Plot bottom" is the lowest
<figure>edge. "Rail content" is the lowest element inside the rail.Overview
epochsInfo.TOPO_PLOT_MIMEnow mirrorsplot_topo(utils.py:163, MNEplot_evoked_topo). It draws a small ERP box at each Muse sensor's position on a head outline: AF7 and AF8 at the front, TP9 and TP10 at the ears. Each box has one line per condition inconditionPaletteorder, and the legend text is in the condition colors. The curves are computed from the same example epochs the ERP tab uses, so the two tabs agree.ERP: graph first, lessons as a walkthrough
ClickableHeadDiagramSVG) and a 2×2 list of sensor buttons (aria-pressed) both select. An Image types legend shows a solid or dashed swatch, the label and the trial count.erpPlot(ERP at TP9, bootstrapped bands, Save as SVG/PNG). The strip below invites the student in: "What am I looking at?" plus Walk me through it. I made MNE step 0 because it is the scientific result the app already computes and saves. The walkthrough is optional teaching layered on the same data. Exit and Finish both return to it.ErpTraceChart, one step at a time in the style ofExploreLessonFlow: a "READING AN ERP · STEP n OF 4" label, gold progress pips, Exit walkthrough ✕, Back (outline) and Next / Finish (filled). The copy fades per step and the heading takes focus.Walkthrough data notes
EXAMPLE_EPOCH_ARRAYSinfixtures.tshas exactly the shape ofpyodide.epochArrays:{ buffer: ArrayBuffer; meta: EpochArraysMeta }, with a float32 C-order buffer laid out [epoch][channel][time].TP9, AF7, AF8, TP10).epochEvents(worker, eventId, -0.1, 0.8). Values are in µV and baseline-corrected.meanTraceandepochChannelSeries(CleanComponent/epochArrays.ts), so no mean is drawn by hand. The MNE-styleerpPlotMime(channel)fixture and the topography fixture are built from the same arrays, so the plot title always matches the selected sensor.EPOCHS_INFOagrees with the arrays: Face 40, House 40, 84 total, 4.76% dropped. The fixture therefore represents P01, and Overview opens with P01 ticked.epochArraysis only requested forraw_epochs(Clean'sloadEpochsEpic). Analyze needsrequestEpochArrays(worker, CLEAN_EPOCHS)afterLoadCleanedEpochs. It also needs either its own state slot or a guarantee that Clean's reviewer is not showing at the same time, because they sharepyodide.epochArrays.Behavior
Response time | AccuracyandBars | Dots | Box, as segmented controls where the choice is outlined in teal, not filled;role="status").aggregateDataForPlot(compute.js, imported read-only). Its input is example CSV rows shaped the wayreadBehaviorDatareturns them: Papa.parse output plusmeta.datafile, with every value a string. So every control changes the plot exactly as it will in the app.compute.jsis not modified.Review agenda
Domain/Analyze, all rendered in the real AppShell with story-accurate badges and Next:ErpExplainerhas been removed; the walkthrough replaces it.Open questions
ClickableHeadDiagramSVGhas no selected state (only:hoverfill) and its<title>reads "Signal Quality Indicator". The sensor buttons carry the selection today. Should integration give the diagram aselectedprop and a correct accessible name?compute.jsacceptsshowDataPointsbut never reads it; Dots is the only way to see points. Should the checkbox be dropped, or should compute be fixed in a separate change?compute.js's hard-coded colors (#28619E/#3DBBDB), notconditionPalette, so Face is a different blue on Behavior than on ERP. Should this be aligned in a separate change?Stray-elements diagnosis (Playtest 1 P0: "Analyze layout broken; stray elements popping up")
These causes are in the current
src/renderer/components/AnalyzeComponent.tsx, for engineering to fix during integration::28importsHelpButtonfromCollectComponent/LessonSidebarand never uses it. It is a leftover of the old sidebar.:79,:81,:285–327: theisSidebarVisibleandhelpModehelp panel.:595, between the tab bar and the content, not as a sidebar. Toggling it pushes the whole tab down; this is the element that "pops up".✕button and an<h1>(:321) that the globalh1 { 36px !important }inflates.handleRemoveOutliersandhandleDisplayModeChangechangehelpModebehind the scenes.:585–592: the "Show help / Hide help" ghost button goes intoSecondaryNavComponent'ssaveButtonslot, assize="icon"(36px square) holding text, so the label overflows its button.isEEGEnabledis not passed either, so the nav renders its legacy layout with a duplicate "Analyze" title.:409: the ERP channel column isw-32(144px at the 18px root).ClickableHeadDiagramSVGhas an inlineminWidth: 250, so the head diagram overflows the column and overlaps the plot.:439–484: the Behavior tab repeats the "EEG Datasets" multi-select and the "No cleaned data yet… Go to Clean" block (:458,:476), so a cleaned-EEG picker and a Clean CTA appear inside Behavior. In a behavior-only workspace this is a Clean reference that should not exist.:579: the root isrelative h-screen bg-appinside AppShell'sflex-1 overflow-y-auto. It is a full window tall under a 64px bar, so it adds a second scroll region and clips the bottom.app.global.cssforcesh1 { 36px !important }andp { 18px !important }, and sets a largeh2. The per-section<h1>s (:332,:398,:442) and<h2>PSD Plot</h2>/<h2>Topography</h2>(:365,:377) render oversized. The new components avoid bare<p>/<h1>or use scoped sizes. Separately,lab.css(layered) styles bareheader, footer, mainwith 24px padding and centered text, so the integration should not wrap Analyze in semantic<main>/<header>without a scoped reset.Constraints not fully met, and notes for integration
AnalyzeComponent.tsxis not replaced. Integration swaps its render branches for these components and removes items 1–6 above.ClickableHeadDiagramSVG.tsxdestructures its props instead of spreading them onto<svg>, which stops React'sonChannelClickunknown-prop warning. This was accepted in round 1 and is otherwise unchanged.PyodidePlotWidget's Save buttons still call preload storage, so they fail in Storybook if clicked. They are not clicked during verification.compute_psd().plot().epochArraysfor cleaned epochs, which Analyze does not request today (see the integration note above).Verification
npx tsc --noEmit: 0 errors./tmp/ws7-design/{1366x768,1280x720}/. They were measured as in the table above: no document scroll, no inner scrollers, no horizontal overflow and 0 console or page errors on any story. The raw numbers are in/tmp/ws7-design/measurements.json.