fix tooltip clipped on single-class chart#1705
Conversation
Vertical BarChart only sets overflow-x-auto on its scroll container. Per the CSS overflow spec, an overflow-x other than 'visible' forces overflow-y to also become 'auto', so the container clips vertically too. With few bars (e.g. a single class) the bar nearly fills the height budget and ECharts' default in-container tooltip renders near the top edge, getting clipped by that implicit vertical overflow. Fix: set appendToBody on the tooltip so ECharts renders it as a position:fixed element attached to document.body, escaping any ancestor overflow containers.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughECharts bar chart tooltips are appended to ChangesBar chart tooltip overflow fix
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Klusha (OpenClaw agent) seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ba15059a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Addresses review feedback from chatgpt-codex-connector on PR #1705: ECharts 6.1.0 (locked frontend dependency) marks appendToBody as deprecated in favor of appendTo: 'body'. Same behavior, non-deprecated API.
|
/review |
❌ Fast Track: checks did not passFailed guardrails: frontend/complexity (Guardrail threw: Cannot find module 'eslint'
Reflects |
What has changed and why?
Fixes LIG-10233: the popover/tooltip is not visible when hovering a class distribution bar chart that has a single class.
Root cause: the vertical
BarChartonly setsoverflow-x-autoon its scroll container (no explicitoverflow-y). Per the CSS overflow spec, once one axis gets a value other thanvisible, the browser forces the other axis to becomeautotoo — so the container clips vertically as well, even though only horizontal scroll was intended. ECharts renders its tooltip by default as a DOM element positioned inside that same container. With a single class, the bar nearly fills the chart's height budget, so the tooltip pops up right at the top edge and gets clipped by that implicit vertical overflow. With several bars of varying height the effect is much less visible, which is why it surfaced specifically on the single-class case.Fix: set
appendToBody: trueon the tooltip inbuildEchartsOption.ts. This is a standard ECharts option that renders the tooltip as aposition: fixedelement attached todocument.body, escaping any ancestor overflow containers.Histogram/buildHistogramOption.tsalready works around a related-but-different issue withconfine: false(keeps the tooltip from being repositioned back inside the viewport) — that option does not address clipping by an ancestor'soverflow.No dependencies added; single-file, ~9-line change.
How has it been tested?
BarChart.svelte/buildEchartsOption.tssource and the existingsingleClassStorybook fixture (BarChart.stories.svelte→ "Single class" story) that reproduces the reported layout.npx prettier --checkon the changed file: clean.npx eslinton the changed file: clean.npm run test:unitfor this file in this environment: it transitively imports the generated API client ($lib/api/lightly_studio_local), which is generated from a running backend (make startinlightly_studio) that wasn't available here. This is pre-existing environment setup, unrelated to the change itself.Did you update CHANGELOG.md?
Summary by CodeRabbit