Scatter chart#4146
Conversation
Pull the inline type of the private `range` field out into a named `AxisRange` interface so the shape can be referenced and reused. No change to behavior or public API. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add `type="scatter"`, which plots each item as a dot at an `[x, y]` coordinate using both axes as value axes. Unlike the evenly-spaced `dot` type, points are positioned absolutely, so they can sit close together, overlap, or stack directly on top of each other. Each item's `value` tuple is read as `[x, y]`. Both axes are auto-ranged (anchored through zero, so negative and zero-crossing data work) and each draws its own grid, zero line, and value badges. `orientation` transposes the plot, and the type honors `displayAxisLabels`, `displayItemText`, and `displayItemValue`. The dot diameter is exposed via the new `--chart-scatter-item-size` custom property. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a `scatter` example (positive data, dot-size and orientation controls) and a `scatter-negative` example (data in all four quadrants, exercising the zero lines and negative ranges on both axes), and wire them into the chart's docs via `@exampleComponent`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add `scatter` to the type selector in the axis-labels example. Because the example's temperature data is one-dimensional, feed scatter a 2D dataset (with matching axis labels) when it is selected, so the labels, item text, and value toggles can be exercised on a real scatter plot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds scatter-chart support to the chart component, including new tuple semantics, axis-range math, scatter item positioning, scatter-specific styles, example datasets and components, and updated chart documentation references. ChangesScatter chart support
Sequence Diagram(s)sequenceDiagram
participant User
participant ChartTypeScatterExample
participant limel-select
participant limel-input-field
participant limel-chart
User->>limel-select: choose orientation
limel-select->>ChartTypeScatterExample: emit change
ChartTypeScatterExample->>limel-chart: set orientation
User->>limel-input-field: change dot size
limel-input-field->>ChartTypeScatterExample: emit change
ChartTypeScatterExample->>limel-chart: set --chart-scatter-item-size
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-4146/ |
Consolidated PR ReviewPR SummaryAdds a Merge Readiness — READY TO MERGE ✅Strictly better than
Dimensions1. Backward Compatibility — SAFE ✅No backward-incompatible changes; everything is additive or behavior-equivalent for existing types. What works well: the Issues: None. 2. Code Quality — GOOD ✅Cohesive helpers and correct edge handling; both previously-flagged What works well: the new private helpers ( Issues: None. Minor nits (4)
3. Architecture — GOOD ✅Scatter fits the existing CSS-variable + per-type-branch architecture coherently; the What works well: reusing the shared Issues: None. Minor nits (2)
4. Security — SAFE ✅No new security issues; presentational component with consistently escaped output. What works well: Issues: None. 5. Observability — GOOD ✅For a presentational component with no logging by design, scatter improves accessible-DOM observability and never crashes. What works well: scatter keeps the full accessible Issues: None. Minor nits (2)
6. Performance — GOOD ✅No performance regressions; the scatter render path is, if anything, slightly cheaper per item than the line/area branches. What works well: Issues: None. Minor nits (2)
Addressed during developmentCaught in an initial pass and already fixed in this branch — listed for the record, not as open work:
Top RecommendationsNone open — the two |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/components/chart/chart.tsx`:
- Around line 597-601: The chart range state is not being recomputed when the
reactive type prop changes, which leaves secondaryAxisRange null for scatter
renders. Add a `@Watch`('type') handler on the Chart component that triggers the
same range reset/recalculation flow used by handleChange and componentWillLoad,
so switching to scatter at runtime recomputes ranges before renderScatterAxises
and getScatterItemStyle run. Use the existing recalculateRangeData,
secondaryAxisRange, and handleChange logic as the reference points when wiring
this up.
In `@src/components/chart/examples/chart-items-scatter.ts`:
- Line 10: The scatter chart example is still typed with the generic ChartItem[]
shape, so it can accept non-scatter scalar values and drift from the intended
[x, y] tuple contract. Update the chartItems declaration in the scatter example
to use the same specialized tuple typing as chart-items-scatter-negative.ts,
specifically Array<ChartItem<[number, number]>>, so the compiler enforces
scatter-only data.
In `@src/components/chart/examples/chart-type-scatter.tsx`:
- Around line 101-103: The handleDotSizeChange handler currently assigns
+event.detail directly, which can turn empty or invalid input into NaN and
propagate an invalid rem value; update this method to validate the parsed value
before mutating dotSize. In chart-type-scatter.tsx, keep the existing dotSize
unchanged unless the parsed number from event.detail is finite and greater than
zero, so the downstream rendering logic does not receive NaN.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: dc5010f3-4ed6-45a1-92af-82b791b2ae61
⛔ Files ignored due to path filters (1)
etc/lime-elements.api.mdis excluded by!etc/lime-elements.api.md
📒 Files selected for processing (12)
src/components/chart/chart.scsssrc/components/chart/chart.tsxsrc/components/chart/chart.types.tssrc/components/chart/examples/chart-axis-labels.tsxsrc/components/chart/examples/chart-items-scatter-negative.tssrc/components/chart/examples/chart-items-scatter.tssrc/components/chart/examples/chart-type-scatter-negative.tsxsrc/components/chart/examples/chart-type-scatter.tsxsrc/components/chart/partial-styles/_axises.scsssrc/components/chart/partial-styles/_display-item-text.scsssrc/components/chart/partial-styles/_layout-for-charts-with-x-y-axises.scsssrc/components/chart/partial-styles/_scatter.scss
| handleChange() { | ||
| this.range = null; | ||
| this.secondaryAxisRange = null; | ||
| this.recalculateRangeData(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add @Watch('type') to avoid a null-deref when type changes to scatter at runtime.
type is a reactive @Prop, but range recalculation only runs in componentWillLoad and on items/axisIncrement/maxValue changes. If type is switched to scatter without those props also changing, secondaryAxisRange is never computed and stays null. The subsequent render reaches getScatterItemStyle → normalize(value, this.secondaryAxisRange) (and renderScatterAxises → renderAxisLines(null)), dereferencing null.totalRange/null.minValue and throwing.
🛡️ Proposed fix
`@Watch`('items')
`@Watch`('axisIncrement')
`@Watch`('maxValue')
+@Watch('type')
handleChange() {
this.range = null;
this.secondaryAxisRange = null;
this.recalculateRangeData();
}Confirm no other mechanism recomputes ranges on type change:
#!/bin/bash
rg -nP "`@Watch`\(|componentWillUpdate|componentWillRender|recalculateRangeData" src/components/chart/chart.tsx🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/chart/chart.tsx` around lines 597 - 601, The chart range state
is not being recomputed when the reactive type prop changes, which leaves
secondaryAxisRange null for scatter renders. Add a `@Watch`('type') handler on the
Chart component that triggers the same range reset/recalculation flow used by
handleChange and componentWillLoad, so switching to scatter at runtime
recomputes ranges before renderScatterAxises and getScatterItemStyle run. Use
the existing recalculateRangeData, secondaryAxisRange, and handleChange logic as
the reference points when wiring this up.
| * or are placed exactly on top of each other, to demonstrate that the scatter | ||
| * type does not space items out the way a dot chart does. | ||
| */ | ||
| export const chartItems: ChartItem[] = [ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Specialize the dataset type to scatter tuples.
ChartItem[] still accepts scalar values, so this file can drift away from the scatter-only [x, y] contract without compiler help. Match src/components/chart/examples/chart-items-scatter-negative.ts and type this as Array<ChartItem<[number, number]>>.
Suggested change
-export const chartItems: ChartItem[] = [
+export const chartItems: Array<ChartItem<[number, number]>> = [📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const chartItems: ChartItem[] = [ | |
| export const chartItems: Array<ChartItem<[number, number]>> = [ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/chart/examples/chart-items-scatter.ts` at line 10, The scatter
chart example is still typed with the generic ChartItem[] shape, so it can
accept non-scatter scalar values and drift from the intended [x, y] tuple
contract. Update the chartItems declaration in the scatter example to use the
same specialized tuple typing as chart-items-scatter-negative.ts, specifically
Array<ChartItem<[number, number]>>, so the compiler enforces scatter-only data.
| private handleDotSizeChange = (event: CustomEvent<string>) => { | ||
| this.dotSize = +event.detail; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject invalid dot sizes before updating state.
Clearing the input or entering an invalid number makes +event.detail become NaN, which then reaches Line 67 as NaNrem. Keep the previous value unless the parsed number is finite and positive.
Suggested change
private handleDotSizeChange = (event: CustomEvent<string>) => {
- this.dotSize = +event.detail;
+ const nextDotSize = Number(event.detail);
+
+ if (Number.isFinite(nextDotSize) && nextDotSize > 0) {
+ this.dotSize = nextDotSize;
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private handleDotSizeChange = (event: CustomEvent<string>) => { | |
| this.dotSize = +event.detail; | |
| }; | |
| private handleDotSizeChange = (event: CustomEvent<string>) => { | |
| const nextDotSize = Number(event.detail); | |
| if (Number.isFinite(nextDotSize) && nextDotSize > 0) { | |
| this.dotSize = nextDotSize; | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/components/chart/examples/chart-type-scatter.tsx` around lines 101 - 103,
The handleDotSizeChange handler currently assigns +event.detail directly, which
can turn empty or invalid input into NaN and propagate an invalid rem value;
update this method to validate the parsed value before mutating dotSize. In
chart-type-scatter.tsx, keep the existing dotSize unchanged unless the parsed
number from event.detail is finite and greater than zero, so the downstream
rendering logic does not receive NaN.
1 participant
Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com
Summary by CodeRabbit
New Features
Bug Fixes
Review:
Browsers tested:
(Check any that applies, it's ok to leave boxes unchecked if testing something didn't seem relevant.)
Windows:
Linux:
macOS:
Mobile: