Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughResponsive legend placement was added to the FacetScatterChart component: component width is measured to set an Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/Compare/FacetScatterChart.vue`:
- Around line 372-409: The mobile teleport target <div
id="compare-scatter-legend-inner"> currently lacks the labelled semantics
present on the desktop target; update that element so it mirrors the desktop
target's accessibility attributes (add role="group" and aria-labelledby pointing
to the same legend label id used by the desktop container) so the Teleported
legend (template `#legend` and Teleport) retains the labelled semantics on mobile;
ensure the referenced label element id exists (or add a visually-hidden label)
to match the desktop aria-labelledby target.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 33dc18dc-5a54-48fc-81d1-820e1d09862c
📒 Files selected for processing (1)
app/components/Compare/FacetScatterChart.vue
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/Compare/FacetScatterChart.vue`:
- Around line 350-355: The component renders two legend containers that both use
role="group" and the same aria-labelledby ("scatter-chart-legend-packages"),
which exposes duplicate/empty groups to assistive tech; locate the div with
id="compare-scatter-legend" and the other legend container that reuses
aria-labelledby="scatter-chart-legend-packages" inside FacetScatterChart.vue and
change the behavior so only a non-empty legend is in the a11y tree—either
conditionally render the empty container (render it only when it has items), or
keep it rendered but add aria-hidden="true" (or remove role/aria-labelledby)
when empty; ensure the remaining visible legend keeps role="group" and the
aria-labelledby value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6e8fdee5-45d2-4153-a4fe-992f3d879eec
📒 Files selected for processing (1)
app/components/Compare/FacetScatterChart.vue
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/Compare/FacetScatterChart.vue (1)
19-23: Keep the JS/mobile switch tied to the same breakpoint source as the layout.
isMobilenow decides which legend container is used, while the layout itself still flips withsm:utilities. Hard-coding640here means the JS and CSS can drift later, leaving the legend in the wrong place for the rendered layout.Also applies to: 310-345
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Compare/FacetScatterChart.vue` around lines 19 - 23, Replace the hard-coded mobile breakpoint (mobileBreakpointWidth = 640) with a value read from a CSS custom property so JS uses the same breakpoint source as the layout: define a global CSS var (e.g. --screen-sm) that matches Tailwind's sm breakpoint in your stylesheet, then in this component read it via getComputedStyle(rootEl).getPropertyValue('--screen-sm') (parseInt) and use that value when computing isMobile; update the symbols mobileBreakpointWidth and isMobile (which use useElementSize and rootEl) to derive the breakpoint from the CSS var instead of the literal 640.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/components/Compare/FacetScatterChart.vue`:
- Around line 285-290: The custom legend slot is currently guarded by
readyTeleport which hides the slot briefly and lets VueUiScatter render its
default legend; remove the v-if from the template slot so the custom legend slot
is always present, and instead add the v-if="readyTeleport" only to the Teleport
element that mounts the legend; keep the readyTeleport shallowRef and onMounted
logic as-is and ensure the slot markup (customLegend / whatever slot name is
used) remains unconditional while only the <Teleport> wrapper is conditional.
---
Nitpick comments:
In `@app/components/Compare/FacetScatterChart.vue`:
- Around line 19-23: Replace the hard-coded mobile breakpoint
(mobileBreakpointWidth = 640) with a value read from a CSS custom property so JS
uses the same breakpoint source as the layout: define a global CSS var (e.g.
--screen-sm) that matches Tailwind's sm breakpoint in your stylesheet, then in
this component read it via
getComputedStyle(rootEl).getPropertyValue('--screen-sm') (parseInt) and use that
value when computing isMobile; update the symbols mobileBreakpointWidth and
isMobile (which use useElementSize and rootEl) to derive the breakpoint from the
CSS var instead of the literal 640.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 699646b7-d367-429a-a238-1584b5d2ab59
📒 Files selected for processing (1)
app/components/Compare/FacetScatterChart.vue
2 participants
Follow up to #2472
This improves the legend of the scatter chart on the compare page, by positioning the items on the side, below the select inputs.
On mobile, the previous behavior is maintained.
One downside: the legend is not part of the png export, however since the chart bears labels above datapoints, we can live with that.