Fix #149: keep the footer consistent on the bubble view - #343
Conversation
The bubble view sized its canvas with a viewport magic number --
"#bubble-view-root { height: calc(100vh - 25rem) }" plus a negative bottom
margin that cancelled ".full.height"'s "--page-space-bottom". Neither number
knows the real height of the navbar, the repo header or the notices above the
graph, so the page came out taller than the viewport (~20px on a 836px-tall
viewport here): the footer was pushed below the fold and sat flush against the
graph, while the same subject in table view had its footer pinned to the bottom
of the viewport with the usual 64px gap above it.
Use the flex layout the landing and auth pages already use (documented at
length in web_src/css/home.css): ".full.height" grows to fill the viewport via
"flex: 1 0 auto", so making it and the wrappers down to the graph flex columns
gives the graph box real free space to grow into. The box takes that space with
"flex: 1 1 0" -- a content-based basis would be circular, since FishboneGraph
sizes its canvas from the box it is given -- floored by a 320px min-height
(MIN_SVG_HEIGHT). The page is then exactly as tall as the viewport, or exactly
as tall as its content when the viewport is too short, and the footer follows
the content in both cases.
The rules live in web_src/css/features/bubble-graph.css instead of the page
template's inline <style>, and are scoped with ":has(...:not([hidden]))" so
they only apply while the bubble view is the visible one.
Verified in the browser against a running instance: the bubble page now ends at
the viewport bottom (836px document height for an 836px viewport) with the
footer at the same offset as the table view, and with the graph forced past the
free space the footer moves down with it instead of overlapping.
Co-authored-by: Pierre Schweiger <schweiger.pierre@gmail.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The four ":has(... :not([hidden]))" selectors were spelled out twice, once for "display: flex" and once for "flex: 1 0 auto", so a rename of the bubble section class had to be made in two places to stay correct. The only reason for the split was ".full.height", which must not lose its own flex value -- but that value is "flex: 1 0 auto" (web_src/css/base.css), the very declaration the second block sets, so merging the blocks changes nothing on the page. Co-authored-by: Pierre Schweiger <schweiger.pierre@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
/review AI review started. |
Advanced AI Review
Click to expand reviewI'll review these changes. First, let me gather context on the affected files and how they interact with the rest of the codebase.Now let me examine the FishboneGraph component's structure and sizing logic, and what gets mounted into I verified the flex chain against the actual DOM ( 1. 🟡 Dead class
|
Review follow-ups on #343, all cleanup on the CSS the fix added: - style the mount point through ".history-bubble-root" instead of "#bubble-view-root". The class was on the element and used nowhere; the ID stays for repo-bubble-view.ts to mount on. Selecting by class keeps the file's specificity uniform at (0,1,0), so a utility class can still win. - drop "overflow: auto" from that box. It was needed when the box had a fixed height its content could exceed; now its only child is "flex: 1 1 0; min-height: 0" and scrolls its own leftover, so the declaration only established a scroll container that can never scroll -- and would clip any overlay painted outside the box. - call the ".secondary-nav" block "the repo header" everywhere instead of alternating with "the article header" six lines apart, and say in the file's top-of-file comment that it now also carries page layout the bubble view needs from its ancestors, which a scoped style cannot reach. - note next to MIN_SVG_HEIGHT that the CSS floor is the same number, so a change to one is not made without the other. Co-authored-by: Pierre Schweiger <schweiger.pierre@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Approved |
Both branches edit the "=== SVG LAYOUT ===" block of FishboneGraph.vue: #343 annotated MIN_SVG_HEIGHT with the page-layout floor it has to stay in sync with, and this branch moved the constant out to graph-viewport.ts so it can be unit-tested. Merging #343 in now so this PR goes onto master cleanly once #343 is merged. Kept this branch's block, and moved #343's cross-reference to where the constant actually lives now. The matching comment in bubble-graph.css pointed at FishboneGraph.vue, which no longer declares it, so it now names graph-viewport.ts too -- otherwise the "change the two together" warning sends the reader to the wrong file. vitest 32/32, vue-tsc, eslint and stylelint all clean on the result. Co-authored-by: Pierre Schweiger <schweiger.pierre@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both conflicts are the cross-reference comments between the bubble graph's canvas floor and the page-layout floor that #343 (Fix #149) and this branch each touched. #343 introduced MIN_SVG_HEIGHT in FishboneGraph.vue and pointed the CSS comment at that file. This branch had already moved the constant to graph-viewport.ts so it could be unit-tested, carrying the same "change the two together" note with it (graph-viewport.ts:11-15). Kept this branch's side in both files: master's `const MIN_SVG_HEIGHT = 320` would now be a duplicate of the exported one, and nothing in FishboneGraph.vue references it directly any more -- canvasHeightFor() applies the floor. The CSS comment now names graph-viewport.ts, which is where the number actually lives after the merge. This is the re-check that reviews/348.md finding 7 asked for once both branches landed. The sizing chain is coherent: .history-bubble-root is "flex: 1 1 0; min-height: 320px" and .f-fishbone-graph is "flex: 1 1 0; min-height: 0; overflow: auto", so the container height comes from the page's flex layout rather than from the canvas measured off it, and the re-measure guards added here cannot drive a feedback loop through it. Co-authored-by: Pierre Schweiger <schweiger.pierre@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* Fix #149: keep the footer consistent on the bubble view The bubble view sized its canvas with a viewport magic number -- "#bubble-view-root { height: calc(100vh - 25rem) }" plus a negative bottom margin that cancelled ".full.height"'s "--page-space-bottom". Neither number knows the real height of the navbar, the repo header or the notices above the graph, so the page came out taller than the viewport (~20px on a 836px-tall viewport here): the footer was pushed below the fold and sat flush against the graph, while the same subject in table view had its footer pinned to the bottom of the viewport with the usual 64px gap above it. Use the flex layout the landing and auth pages already use (documented at length in web_src/css/home.css): ".full.height" grows to fill the viewport via "flex: 1 0 auto", so making it and the wrappers down to the graph flex columns gives the graph box real free space to grow into. The box takes that space with "flex: 1 1 0" -- a content-based basis would be circular, since FishboneGraph sizes its canvas from the box it is given -- floored by a 320px min-height (MIN_SVG_HEIGHT). The page is then exactly as tall as the viewport, or exactly as tall as its content when the viewport is too short, and the footer follows the content in both cases. The rules live in web_src/css/features/bubble-graph.css instead of the page template's inline <style>, and are scoped with ":has(...:not([hidden]))" so they only apply while the bubble view is the visible one. Verified in the browser against a running instance: the bubble page now ends at the viewport bottom (836px document height for an 836px viewport) with the footer at the same offset as the table view, and with the graph forced past the free space the footer moves down with it instead of overlapping. Co-authored-by: Pierre Schweiger <schweiger.pierre@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor: describe the bubble view's flex chain in one rule The four ":has(... :not([hidden]))" selectors were spelled out twice, once for "display: flex" and once for "flex: 1 0 auto", so a rename of the bubble section class had to be made in two places to stay correct. The only reason for the split was ".full.height", which must not lose its own flex value -- but that value is "flex: 1 0 auto" (web_src/css/base.css), the very declaration the second block sets, so merging the blocks changes nothing on the page. Co-authored-by: Pierre Schweiger <schweiger.pierre@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix #348: re-measure the bubble graph when its container resizes The <svg> was sized from the container at mount and then only corrected by a ResizeObserver whose bookkeeping could not tell a real resize from a redelivery: it stored the CLAMPED layout width (min(w, 1100)) as "the last width seen" and compared the next raw measurement against it, so every delivery on a container wider than 1100px re-ran the layout and reset the user's pan/zoom, while the mount path adopted the raw width and drew the first frame at a width no later frame would use. Measurement is now one path. graph-viewport.ts holds the pure functions (canvas height, clamped layout width, measurable/changed tests) with unit tests; the component keeps the RAW box it last measured, so the bail-out compares like with like and an unchanged container costs one rect read and stops. The observer stays on the CONTAINER, never on the <svg> it sizes, so a taller canvas cannot enlarge what is observed, and it is disconnected (with any pending rAF cancelled) on unmount. Two triggers are added for the deliveries the observer cannot make. A window resize and the tab returning to the foreground re-measure, because a background tab's rendering is throttled and the resize that happened there is coalesced away. And repo-history.ts dispatches `repo:bubble-visible` after the switch to bubble view: the component mounts as part of that switch, when its section can still be the hidden 0-height placeholder, so the box it measured at mount is not the box it is drawn in. Both run the same bail-out and do nothing when nothing moved. Co-authored-by: Pierre Schweiger <schweiger.pierre@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor: style the bubble view root by class and drop its dead overflow Review follow-ups on #343, all cleanup on the CSS the fix added: - style the mount point through ".history-bubble-root" instead of "#bubble-view-root". The class was on the element and used nowhere; the ID stays for repo-bubble-view.ts to mount on. Selecting by class keeps the file's specificity uniform at (0,1,0), so a utility class can still win. - drop "overflow: auto" from that box. It was needed when the box had a fixed height its content could exceed; now its only child is "flex: 1 1 0; min-height: 0" and scrolls its own leftover, so the declaration only established a scroll container that can never scroll -- and would clip any overlay painted outside the box. - call the ".secondary-nav" block "the repo header" everywhere instead of alternating with "the article header" six lines apart, and say in the file's top-of-file comment that it now also carries page layout the bubble view needs from its ancestors, which a scoped style cannot reach. - note next to MIN_SVG_HEIGHT that the CSS floor is the same number, so a change to one is not made without the other. Co-authored-by: Pierre Schweiger <schweiger.pierre@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix #348: compare the width the layout runs at, and keep a moved view Review follow-up on #352. sizeChanged() compared RAW widths, but nothing downstream consumes a raw width: every layout input comes from the CLAMPED one. So a horizontal resize between two widths above the cap (1200px -> 1300px) reported "changed" and cost a full re-layout plus resetView() for a change no layout input can see. Both sides now go through layoutWidthFor(). A genuine resize no longer re-frames a view the user has moved. The re-measure path re-centres at 1:1, which is right for a view still where the graph put it and wrong for a panned or zoomed one — and this branch added three more ways to reach it. A d3 zoom event carrying a sourceEvent marks the view as the user's; resetView() takes it back. The kept branch still refreshes the pan bound and the zoom floor, which resetView() would otherwise have done on its way past. The re-measure triggers move into graph-viewport.ts (registerRemeasureTriggers / observeContainerResize) and are registered at the top of onMounted, before any await. The repo:bubble-visible handoff no longer depends on this mount's awaits resolving ahead of the dispatcher's — it worked only by microtask FIFO and would have broken silently the first time an await was added above. The wiring now has unit tests, which is where the bug lived; the event name is one exported constant. Also: WIDTH_BREAKPOINT_MAX is MAX_LAYOUT_WIDTH, since every path clamps now and a breakpoint above the clamp is unreachable (the widest screen saturated at 0.86 and the wide end of every dial never engaged); ensureBubbleView() claims the mount before its await, so the two callers racing on the first switch cannot both get through; resetView()'s zero-box retry goes through pendingRaf so unmount cancels it; and the stale comment about the empty state in scheduleRemeasure() is corrected. Co-authored-by: Pierre Schweiger <schweiger.pierre@gmail.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closes #149
Problem
The footer sat in a different vertical position on the bubble view than on the table view (and every other page).
The bubble view claimed its vertical space with a viewport magic number in the page template's inline
<style>:25rem(400px) does not know the real height of the navbar, the repo header or the notices above the graph, and the negative bottom margin cancelled the--page-space-bottomgap that.full.heightreserves above the footer everywhere else. Measured against a running instance (836px viewport):So the bubble page overflowed the viewport by ~20px, the footer had to be scrolled to, and it was flush against the graph.
Fix
Stop measuring the viewport; participate in the flex layout instead — the same solution
web_src/css/home.cssdocuments at length for the landing page (#316 / #242 / #276)..full.heightalready grows to fill the viewport viaflex: 1 0 auto, so making it and the wrappers down to the graph flex columns gives the graph box real free space to grow into:#bubble-view-roottakes that space withflex: 1 1 0.flex-basis: 0is deliberate —FishboneGraphsizes its canvas from the box it is given, so a content-based basis would be circular — floored bymin-height: 320px, which isMIN_SVG_HEIGHTin the component (a higher floor would re-create the overflow)..f-fishbone-graphloses its owncalc(100vh - 25rem)and fills the box (flex: 1 1 0; min-height: 0), so anything left over scrolls inside the graph rather than growing the page past the footer.<style>intoweb_src/css/features/bubble-graph.css, scoped with:has(… :not([hidden]))so they only apply while the bubble view is the visible one (the table/article sections stay untouched).No new percentage heights, no
100vharithmetic to keep in sync with the header.Verification
Verified in Chrome against a running instance (
localhost:3000, subjectexample-subject), by measuring the layout with and without the new rules applied:svg height: 800pxseen when switching table → bubble is pre-existing: it reproduces identically on the unpatched build, and is unrelated to this change.make lint-csspasses.FishboneGraph.vueis outside its file glob; running stylelint on it directly reports the same 28 pre-existing errors before and after this change.Model used: Claude Opus 5 (1M context).