fix(lcms): fit the three-graph stack, toolbar and panel to their container - #325
Merged
Conversation
The LC/MS layout is the only place the editor mounts three chart containers
stacked in one pane (.d3Line for UV/VIS, .d3Multi for the TIC, .d3Rect for the
m/z scan); every other layout mounts exactly one. A host stylesheet that
stretches a single chart with `.d3Line { height: 100% }` therefore makes each of
the three as tall as the whole pane, and a bounded, `overflow: hidden` host
container clips everything after the first - the editor renders the UV/VIS
chromatogram and silently drops the TIC and m/z graphs.
Make the stack root a flex column with `min-height: 0`, so that same declaration
becomes a flex-basis the three panes negotiate down to a third each and the
stack fits either way. The panes were already drawn for this: H is
`window.innerHeight * 0.9 * 0.8 / 3` and the svg keeps its aspect ratio, so
forcing `height: 100%` on a mount only letterboxes it.
The toolbar had the same shape of problem: its outlined selects are compressed
to `selectInput.height = 30`, well under MUI's outlined geometry, so a shrunk
InputLabel ("Submit", "Write Peaks", "Write Intensity", "Decimal") floats to
about -9px, outside its own box. That rendered only as long as nothing above
clipped. Reserve the room with a top padding on the toolbar card so it holds for
any host.
Neither is fully fixable from here, because withStyles emits opaque class names
(`jss8 jss4`) that a host cannot target. Add LIST_HOST_HOOK_CLASS - `rse-cmd-bar`,
`lcms-stack`, `lcms-graph-panel` - as stable hooks alongside them, document them
in the host-integration contracts, and pin the literal strings in a test so a
rename has to be deliberate and host-coordinated.
refs: ComPlat/chemotion_ELN#3477
7 tasks
The right-hand accordion panel (Info, Multiplicity, Peaks, …) is truncated at the bottom once its content is tall, with no way to reach the rest. `panels` declared `display: table` above `max-height` and `overflow-y: auto`, and a table box honours neither: CSS leaves the effect of `max-height` on a table undefined, and `overflow` does not make one a scroll container. Both declarations were dead. Measured in Chrome with five expanded accordions, the panel renders 1512px tall inside a 748px column - it never capped, never scrolled, and simply overflowed whatever contained it. That was invisible for as long as the host let the page grow around it. A host that bounds the editor and clips the overflow instead - as chemotion_ELN now does - shows the panel cut off at the bottom, which is the reported symptom. Make it a block box, and take the height from a bounded parent via `height: 100%` + `min-height: 0`. Against an unbounded parent `height: 100%` computes to `auto`, so the existing viewport-derived `max-height` still governs there - and now actually applies, capping the same content at 674px with a working scrollbar instead of 1512px without one. Verified in headless Chrome across both cases (bounded host column and unbounded standalone), at 1920x1080 and 1366x768.
Follow-up on this branch's own review, most of it caught by measuring the layout in a browser rather than by reading it. Pane heights. Giving the m/z wrapper `flex: 1 1 auto` while a host puts `height: 100%` on the two bare chart mounts left the three panes with different flex bases, so the shrink was shared in proportion to basis and m/z came out at half the height of its siblings (measured 130px against 258px). Restating `height: 100%` on all three from here makes the bases agree: 215/215/215. Not `flex-basis: 0`, which was the first attempt and was worse - a host stylesheet loads after this JSS and chemotion_ELN's rule for `.d3Line`/`.d3Multi` outranks anything reachable from a JSS child selector, so a basis of 0 applied to the m/z panel alone and collapsed it to zero height. Measured, not reasoned: the regression only showed up on re-running the browser probe. This also makes the stack self-contained. It previously depended on the host supplying `flex`/`min-height` to the two mounts; now a host that merely bounds the editor's height gets three even panes without shipping a rule for them, and the companion `spectra.scss` change becomes an optimisation rather than a prerequisite. Hook contract: - Prefix the new hooks (`rse-lcms-stack`, `rse-lcms-graph-panel`). They land in a host's global, non-modular stylesheet next to its own classes, and the no-rename rule makes a generic name expensive to undo later. - Add `EDITOR_ROOT` for `react-spectrum-editor` - the oldest hook and the one hosts actually target, previously a bare string in four components and absent from the contract it belongs to. Keeps its historical unprefixed name. - Export `LIST_HOST_HOOK_CLASS` from the package entry point, so a host can reference the strings instead of deep-importing dist/ or hard-coding them. Toolbar budget: `commonStyle.card` now reserves 10px, so the two layouts that size against a hard-coded constant give it back - `calc(90vh - 220px)` becomes `230px` in the CV editor and the info panel. Without that they overflow by exactly the padding. Tests: assert the hook names per entry rather than with `toEqual` on the whole object, so adding a fourth hook is not a breaking change; drop a tautological case; remove an `hplcMsSt` prop that react-redux's default mergeProps discards; fix the eslint offences in the new file and the misleading comment about `beforeAll` scope.
Introduced when the bare 'react-spectra-editor' literal was replaced with LIST_HOST_HOOK_CLASS.EDITOR_ROOT, which pushed the line to 105 characters. Caught by CI's cypress job, not by 'yarn test': the production build runs eslint and fails on it, while the unit-test command does not lint at all. Verified here by running the same production build rather than eslint alone, whose standalone config cannot parse this codebase's optional chaining.
…her controls Two reports against the LC/MS layout. The threshold buttons above the m/z graph sat in the toolbar row's right cluster, alone, while every other control in all three rows sits left. That put them far from the buttons they belong with and nowhere near the graph they act on. Moved into the left cluster beside that pane's own zoom control; the right cluster is now empty, matching the two rows above. The graphs also left part of the pane unused, more so the wider the viewport. Each chart svg carries preserveAspectRatio="xMinYMin meet", so the viewBox aspect - not the container - decides how much of the pane the drawing fills, and the viewBox came from a W/H pair computed once from window.innerWidth at module load. Whenever the real pane is proportionally wider than that ratio, the drawing scales down to the pane height and the surplus width is left empty on the right. Measured in Chrome against the host's DOM chain, with the stack sized the way the modal sizes it, the waste was 17% at 1920x1080, 17% at 2560x1080 and 6% at 3440x1440 - so this was never only an ultrawide problem, just most visible there. Note it is driven by the pane being shorter than the H estimate, which depends on the host's modal chrome; the percentages carry that assumption, the mechanism does not. Measure each pane and give its svg a matching viewBox, remounting on resize - the pattern d3_multi already uses for the Cyclic Voltammetry layout, which is the only layout that measured its container until now. Drawn width is then the full pane width in every case above. Falls back to the previous constants when a pane cannot be measured, which keeps jsdom (clientWidth 0) and any pre-paint call on exactly today's behaviour. Only the first mount resets redux; a resize must not discard the user's zoom, threshold or selection. Redraw is guarded on a real size change, so against an unbounded host - where the measured size is the one the current viewBox already produces - it settles after the first pass instead of feeding itself. The layout effect itself is not unit-testable (jsdom reports no geometry), so the tests cover the measurement logic and its fallback, and the fill claim rests on the browser measurement above.
The pane-measuring resize path failed the whole LC/MS e2e suite. Two distinct bugs, both invisible to the unit tests and to the production build, and both caught only by running cypress. First: "ResizeObserver loop completed with undelivered notifications", which Cypress reports as an uncaught application error rather than a console warning. Remounting synchronously inside the observer callback resizes the very subtree being observed. I had reasoned this was a stable fixed point and it is not - a pane whose height comes from its content takes that height from the svg, whose height comes back from the viewBox the callback is setting. d3_multi does remount synchronously, but its resize path is gated to Cyclic Voltammetry, whose container height is fixed in CSS and so cannot be fed back into; this stack has no such guarantee, and the standalone demo the e2e suite drives is exactly the unbounded case. Defer to the next animation frame so the observer finishes delivery before anything moves, and require a change of more than a pixel before remounting at all, so integer round-trip noise cannot retrigger it. Second, uncovered once the first was fixed: the remount recreated the m/z pane with an empty series. That pane is populated by componentDidUpdate, not by the mount path, so a resize wiped the displayed scan and nothing redrew it - the graph went blank, and `.d3Rect .d3SvgRect path[d]` stopped existing. Seed it from the currently selected sub-view instead, which is still empty on first mount and correct on every remount after. Verified by running the LC/MS spec locally in a container (the host's port 3000 is in use by another service, and the spec hardcodes that URL): 7/7 pass, and the full suite is 81/81 across all ten specs, including the Cyclic Voltammetry and HPLC layouts that share the toolbar card and editor root this branch also touches.
PiTrem
changed the title
PiTrem
pushed a commit
that referenced
this pull request
Aug 26, 2026
TIC and UV/VIS time axes disagreed by a factor of 60 whenever a TIC arrived in
seconds. extrSpectraMs now resolves a seconds-to-minutes scale for TIC data the
same way it already did for UVVIS - an explicit-minutes veto, an explicit-seconds
signal, and a magnitude check on the data itself - and applies it in the
isTicData branch, so entity.spectra carries minutes and every consumer of it
agrees rather than only the display projection.
Both LC/MS panes also auto-fitted their x-domain to their own data, so they
started out misaligned before any zoom. The union of the two ranges is now
seeded into sweepExtent[0]/[1] through the existing lcmsSyncX mirroring in
updateZoom, rather than through a second pane-sync mechanism, and only when both
extents are unset and both sides have data - a partial union would otherwise get
stuck once the missing side arrived.
Also extracts resolveXExtent, which drops a dead sort after d3.extent and gives
the degenerate empty-data case a fixed placeholder range instead of letting
{ xL: undefined, xU: undefined } reach scales.x.domain() as NaN.
Rebased onto master after #325. That PR restructured componentDidMount into
mountCharts(sizes, shouldReset) so the panes can be measured and remounted on
resize, so the union seed moves inside mountCharts at the same point in the
sequence, and the chart mounts take their measured sizes. Two consequences of
the rebase, both verified rather than assumed: the wiring test now installs its
focus stubs from createFocuses, since mountCharts rebuilds those objects every
time it runs; and componentDidUpdate no longer assumes mountCharts has already
set currentSizes, measuring instead of dereferencing null.
dist/ regenerated from the merged source.
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.
Problem
Opening a genuine LC/MS dataset in the ELN Spectra Editor renders only the first (UV/VIS) graph — the TIC and m/z panes are missing — and the header toolbar's outlined selects (
Submit,Write Peaks, …) are truncated.The LC/MS data path is fine. Verified against a real converter-produced trio (
*_lcms_tic.jdx,*_lcms_uvvis.jdx,*_lcms_mz.jdx):isChemstationLcmsExtractJcamp().layoutLC/MSLC/MSLC/MSgetLcMsInfo().kindticuvvismzsplitAndReindexEntitiesbuckets them 1/1/1 andisLcMsGroupis true. The panes are built; they are not visible.Cause
LC/MS is the only layout that mounts three chart containers stacked in one pane (
.d3LineUV/VIS,.d3MultiTIC,.d3Rectm/z,src/components/d3_line_rect/index.js); every other layout mounts exactly one. chemotion_ELN'sspectra.scss(added by ComPlat/chemotion_ELN#3477, the fullscreen-modal fix) stretches a single chart withwhich is right for the single-chart layouts and makes the LC/MS stack ~3× its container. The same PR's
overflow: hiddenthen clips everything after the first chart. Its own test plan lists LC-MS as not verified live.The same
overflow: hiddenclipsCmdBar, which sits above.react-spectrum-editor. Its outlined selects are compressed toselectInput.height = 30, well under MUI's outlined geometry, so a shrunkInputLabelfloats to about-9px— outside its own box. That only ever rendered because nothing above clipped.Changes
lcMsStackRootbecomes a flex column withmin-height: 0(toolbar rowsflex-shrink: 0,lcMsGraphPanela proper flex item). A hostheight: 100%then acts as a flex-basis the three panes negotiate down to a third each, so the stack fits with or without the host rule. The panes were already drawn for this —H = window.innerHeight * 0.9 * 0.8 / 3withpreserveAspectRatio="xMinYMin meet", so forcingheight: 100%on a mount letterboxes it rather than enlarging it.LIST_HOST_HOOK_CLASS(src/constants/list_graph.js) —rse-cmd-bar,lcms-stack,lcms-graph-panel— stable class names beside the opaquewithStylesones (jss8 jss4), which a host stylesheet cannot target. Documented under Host Integration Contracts indocs/architecture/frontend-architecture.md.dist/recompiled so a chemotion_ELN branch can depend on this branch directly from GitHub.No version bump and no release — the ELN fix branch consumes the branch while both sides are verified together.
Also here: the right-hand panel never scrolled
Reported separately while testing this branch — the Info / Multiplicity accordion column is truncated at the bottom once its content is tall, with no way to reach the rest.
Same family, different node, and this one is a pre-existing bug in this package rather than anything the host did.
PanelViewer'spanelsrule declareddisplay: tableabovemax-heightandoverflow-y: auto, and a table box honours neither — CSS leaves the effect ofmax-heighton a table undefined, andoverflowdoes not make one a scroll container. Both declarations were dead everywhere, in every host, since they were written.Measured in headless Chrome with five expanded accordions:
display: table(today)display: block+height: 100%It stayed invisible for as long as the host let the page grow around it. A host that bounds the editor and clips the overflow shows it as truncation instead.
The fix takes the height from a bounded parent (
height: 100%+min-height: 0); against an unbounded parent that computes toauto, so the existing viewport-derivedmax-heightstill governs the standalone case — and now actually applies, capping the same content at 674px with a working scrollbar instead of 1512px without one.Test plan
yarn test— 78 suites / 664 tests pass (built and run in the pinnednode:22.23.1image), at both commits.src/__tests__/units/components/host_dom_hooks.test.jspins the published class strings and asserts the three chart mounts really are inside.lcms-stack, so a rename or a DOM reshuffle has to be a deliberate, host-coordinated change.vis=0 / vis=0for TIC and m/z to all three visible, the toolbar's floating label moves from-7pxto+3px, and the right-hand panel gains a working scrollbar. Notably the RSE change alone is sufficient for the panes — the companionspectra.scsschange makes the three panes equal thirds rather than leaving m/z at half height, and on its own it is a no-op, since it keys on hook classes only this PR emits.spectra.scsschange — three panes visible, no modal scrollbar,Submitand its label readable. Regression-check NMR (1H/13C), Cyclic Voltammetry (shares.react-spectrum-editorwith its owncalc(90vh - 220px)sizing) and oneMultiJcampsViewerlayout.refs: ComPlat/chemotion_ELN#3477