Remove default container width for altair charts#8696
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
51a4b1d to
7ce3b6f
Compare
There was a problem hiding this comment.
Pull request overview
Removes Marimo’s prior behavior of forcing Altair/Vega-Lite charts to stretch to the notebook container width by default, aligning rendered output more closely with Altair defaults (Fixes #8596).
Changes:
- Stop mutating Altair specs to apply
width="container"and related vconcat autosize adjustments. - Update Vega embed styling so full-width rendering only happens when the spec explicitly uses
width: "container". - Adjust tests to reflect the new default-width behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
marimo/_plugins/ui/_impl/altair_chart.py |
Removes default full-width/vconcat sizing mutations from mo.ui.altair_chart. |
marimo/_output/formatters/altair_formatters.py |
Stops forcing width="container" in the Altair output formatter. |
frontend/src/plugins/impl/vega/vega.css |
Makes full-width styling conditional via .vega-container-width. |
frontend/src/plugins/impl/vega/vega-component.tsx |
Adds conditional .vega-container-width wrapper class based on spec.width === "container". |
frontend/src/plugins/impl/data-explorer/ConnectedDataExplorerComponent.tsx |
Ensures the data explorer’s container-width charts still expand by applying .vega-container-width. |
tests/_plugins/ui/_impl/test_altair_chart.py |
Removes tests tied to old width/autosize mutation logic; adds a new default-autosize expectation. |
tests/_output/formatters/test_altair_formatters.py |
Updates formatter test to assert default width isn’t forcibly applied. |
Comments suppressed due to low confidence (1)
frontend/src/plugins/impl/vega/vega-component.tsx:318
vega-container-widthis applied to the wrapper<div>here, but the element thatuseVegaEmbedpopulates is the nested<div ref={vegaRef} />. If the generated.vega-embedends up inside that ref element (common for vega-embed), the CSS rule.vega-container-width > .vega-embedwon’t match and container-width charts may not actually expand. Consider movingvega-container-widthonto thevegaRefelement (or adjusting the CSS selector to target descendants) so width=container reliably results in full-width rendering.
<div
className={cn(
"relative",
"width" in selectableSpec &&
selectableSpec.width === "container" &&
"vega-container-width",
)}
// Capture the pointer down event to prevent the parent from handling it
onPointerDown={Events.stopPropagation()}
>
<div ref={vegaRef} />
{renderHelpContent()}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
manzt
left a comment
There was a problem hiding this comment.
Thanks! Just a couple of minor comments.
| @media (min-width: 500px) { | ||
| min-width: 300px; | ||
| } | ||
| max-width: 100%; |
There was a problem hiding this comment.
I think this may be redundant / already set with vega-embed-wrapper but probably good to duplicate max-width here as a guard in case the wrapper isn't present (e.g., shadow DOM or custom rendering). Maybe add a comment why?
| className="relative" | ||
| className={cn( | ||
| "relative", | ||
| "width" in selectableSpec && |
There was a problem hiding this comment.
I'm wondering if we should have some explicit flag that "this was set by us" instead of inspecting the spec which could come from a user or us? Might be overkill.
|
🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.20.5-dev88 |
|
Thank you for this! |
📝 Summary
Fixes #8596.
Frontend:
.vega-embedno longer gets blanketwidth: 100%; usesmax-width: 100%insteadvega-container-widthCSS class restoreswidth: 100%only when the spec explicitly useswidth: "container"(used by the data explorer and respected for user-set specs)Backend:
maybe_make_full_widthcalls from the Altair formatter andmo.ui.altair_chart🔍 Description of Changes
📋 Checklist