feat(chart): have legend #3724
feat(chart): have legend #3724
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdded a legend UI component to the chart with visibility toggle functionality. Implemented state management to track hidden items, added legend rendering logic, and updated range/axis calculations to reflect only visible items. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Legend
participant Chart Component
participant State
participant Rendering
User->>Legend: Click legend item
Legend->>Chart Component: handleLegendClick(item)
Chart Component->>State: Toggle item in hiddenItems
State->>Chart Component: Update state
Chart Component->>Chart Component: calculateRange(visibleItems only)
Chart Component->>Chart Component: axisIncrement(visibleItems only)
Chart Component->>Rendering: renderItems (filtered)
Rendering->>Rendering: renderLegend (with updated visibility)
Rendering-->>User: Chart re-rendered with item hidden/shown
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/chart/chart.tsx (2)
146-161: Use<Host>instead of array literal for multiple JSX elements.StencilJS components should wrap multiple top-level JSX elements in the
<Host>component rather than an array literal.As per coding guidelines.
First, add
Hostto the imports at the top of the file:import { Component, Event, EventEmitter, h, + Host, Prop, State, Watch, } from '@stencil/core';Then, apply this diff to wrap the elements in
<Host>:- return [ + return ( + <Host> <table aria-busy={this.loading ? 'true' : 'false'} aria-live="polite" style={{ '--limel-chart-number-of-items': this.items.length.toString(), }} > {this.renderCaption()} {this.renderTableHeader()} {this.renderAxises()} <tbody class="chart">{this.renderItems()}</tbody> - </table>, - this.renderLegend(this.items), - ]; + </table> + {this.renderLegend(this.items)} + </Host> + );
18-44: Consider adding a documentation example for the legend feature.The legend is a significant new feature that would benefit from a dedicated example to demonstrate its behavior and help users understand when and how it appears.
Based on learnings.
Consider creating a new example file (e.g.,
limel-example-chart-legend.tsx) and adding it to the component documentation:* @exampleComponent limel-example-chart-clickable-items * @exampleComponent limel-example-chart-accessibility + * @exampleComponent limel-example-chart-legend * @exampleComponent limel-example-chart-styling
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
src/components/chart/chart.scss(2 hunks)src/components/chart/chart.tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.tsx
📄 CodeRabbit inference engine (.cursor/rules/wrap-multiple-jsx-elements-in-host-not-in-array.mdc)
When returning multiple JSX elements from the
rendermethod, never wrap them in an array literal. Instead, always wrap them in the special<Host>element. When there is already a single top-level element, it does not have to be wrapped in<Host>.
Files:
src/components/chart/chart.tsx
⚙️ CodeRabbit configuration file
**/*.tsx: Our.tsxfiles are typically using StencilJS, not React. When a developer wants to return multiple top-level JSX elements from therendermethod, they will sometimes wrap them in an array literal. In these cases, rather than recommending they addkeyproperties to the elements, recommend removing the hardcoded array literal. Recommend wrapping the elements in StencilJS's special<Host>element.
Files:
src/components/chart/chart.tsx
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: Imports from other files in the same module (lime-elements) must use relative paths. Using absolute paths for imports will cause the production build to fail.
Files:
src/components/chart/chart.tsx
**/*.{tsx,scss}
⚙️ CodeRabbit configuration file
**/*.{tsx,scss}: Almost all our components use shadow-DOM. Therefore, we have no need of BEM-style class names in our CSS.
Files:
src/components/chart/chart.tsxsrc/components/chart/chart.scss
src/components/**/*.tsx
⚙️ CodeRabbit configuration file
src/components/**/*.tsx: When contributors add new props to existing components in the lime-elements repository, they should always add documentation examples that demonstrate the new prop's usage and explain how it works. This helps with user adoption, feature discoverability, and maintains comprehensive documentation.
Files:
src/components/chart/chart.tsx
src/components/**/*.scss
📄 CodeRabbit inference engine (.cursor/rules/css-class-names-in-components-using-shadow-dom.mdc)
Do not use BEM-style class names in CSS for components, as components use shadow-DOM.
Files:
src/components/chart/chart.scss
🧠 Learnings (1)
📚 Learning: 2025-04-17T09:39:36.254Z
Learnt from: adrianschmidt
Repo: Lundalogik/lime-elements PR: 3530
File: src/components/text-editor/examples/text-editor-composite.tsx:4-7
Timestamp: 2025-04-17T09:39:36.254Z
Learning: For lime-elements, example files should import types from the public API using 'limetech/lime-elements' rather than defining duplicate types locally. This includes types like `EditorUiType` which are part of the component's public API.
Applied to files:
src/components/chart/chart.tsx
🧬 Code graph analysis (1)
src/components/chart/chart.tsx (2)
src/util/random-string.ts (1)
createRandomString(1-10)src/components/chart/chart.types.ts (1)
ChartItem(7-38)
🪛 Biome (2.1.2)
src/components/chart/chart.tsx
[error] 274-281: Static Elements should not be interactive.
To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value.
(lint/a11y/noStaticElementInteractions)
[error] 274-281: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Docs / Publish Docs
- GitHub Check: Test
- GitHub Check: Build
🔇 Additional comments (6)
src/components/chart/chart.scss (1)
22-23: LGTM! Vertical layout for chart and legend.The flex-direction and gap properties appropriately stack the chart and legend vertically.
src/components/chart/chart.tsx (5)
1-9: LGTM! State import added correctly.The
Stateimport is necessary for the newhiddenItemsstate management.
128-129: LGTM! Appropriate state management for hidden items.Using a
Set<string>efficiently tracks hidden items with O(1) lookup performance.
227-258: LGTM! Filtering logic correctly excludes hidden items.The filter properly checks
hiddenItemsbefore rendering, ensuring only visible items are displayed.
302-316: LGTM! Toggle logic and range recalculation are correct.The method properly creates a new Set for immutability, toggles visibility, and recalculates the range to reflect the visibility change.
427-470: LGTM! Range calculation correctly accounts for hidden items.The range calculation appropriately filters to visible items and includes a fallback to prevent issues when all items are hidden.
| .legend { | ||
| display: flex; | ||
| align-self: center; | ||
| max-width: 80%; | ||
| font-size: small; | ||
|
|
||
| background-color: rgb(var(--contrast-500)); | ||
| border-radius: 0.5rem; | ||
| padding: 0.5rem; | ||
|
|
||
| .legend-content { | ||
| display: flex; | ||
| width: 100%; | ||
| overflow-x: auto; | ||
| gap: 1rem; | ||
|
|
||
| @include mixins.fade-out-overflowed-content-on-edges(horizontally); | ||
| } | ||
| } | ||
|
|
||
| .legend-item { | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 0.25rem; | ||
| cursor: pointer; | ||
| user-select: none; | ||
| white-space: nowrap; // Prevent text wrapping | ||
| flex-shrink: 0; // Prevent items from shrinking | ||
|
|
||
| &--hidden { | ||
| opacity: 0.5; | ||
| } | ||
|
|
||
| limel-badge { | ||
| transition: | ||
| opacity 0.2s, | ||
| filter 0.2s; | ||
| } | ||
|
|
||
| &:hover limel-badge { | ||
| filter: brightness(1.2); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Avoid BEM-style class names in shadow-DOM components.
The .legend-item--hidden uses BEM-style naming, which is unnecessary since this component uses shadow-DOM for style scoping.
As per coding guidelines.
Apply this diff to simplify the class naming:
-.legend-item {
+.legend-item {
display: flex;
align-items: center;
gap: 0.25rem;
cursor: pointer;
user-select: none;
white-space: nowrap; // Prevent text wrapping
flex-shrink: 0; // Prevent items from shrinking
- &--hidden {
+ &.hidden {
opacity: 0.5;
}
limel-badge {
transition:
opacity 0.2s,
filter 0.2s;
}
&:hover limel-badge {
filter: brightness(1.2);
}
}Then update the corresponding class usage in src/components/chart/chart.tsx at line 277:
class={{
'legend-item': true,
- 'legend-item--hidden': isHidden,
+ 'hidden': isHidden,
}}📝 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.
| .legend { | |
| display: flex; | |
| align-self: center; | |
| max-width: 80%; | |
| font-size: small; | |
| background-color: rgb(var(--contrast-500)); | |
| border-radius: 0.5rem; | |
| padding: 0.5rem; | |
| .legend-content { | |
| display: flex; | |
| width: 100%; | |
| overflow-x: auto; | |
| gap: 1rem; | |
| @include mixins.fade-out-overflowed-content-on-edges(horizontally); | |
| } | |
| } | |
| .legend-item { | |
| display: flex; | |
| align-items: center; | |
| gap: 0.25rem; | |
| cursor: pointer; | |
| user-select: none; | |
| white-space: nowrap; // Prevent text wrapping | |
| flex-shrink: 0; // Prevent items from shrinking | |
| &--hidden { | |
| opacity: 0.5; | |
| } | |
| limel-badge { | |
| transition: | |
| opacity 0.2s, | |
| filter 0.2s; | |
| } | |
| &:hover limel-badge { | |
| filter: brightness(1.2); | |
| } | |
| } | |
| .legend { | |
| display: flex; | |
| align-self: center; | |
| max-width: 80%; | |
| font-size: small; | |
| background-color: rgb(var(--contrast-500)); | |
| border-radius: 0.5rem; | |
| padding: 0.5rem; | |
| .legend-content { | |
| display: flex; | |
| width: 100%; | |
| overflow-x: auto; | |
| gap: 1rem; | |
| @include mixins.fade-out-overflowed-content-on-edges(horizontally); | |
| } | |
| } | |
| .legend-item { | |
| display: flex; | |
| align-items: center; | |
| gap: 0.25rem; | |
| cursor: pointer; | |
| user-select: none; | |
| white-space: nowrap; // Prevent text wrapping | |
| flex-shrink: 0; // Prevent items from shrinking | |
| &.hidden { | |
| opacity: 0.5; | |
| } | |
| limel-badge { | |
| transition: | |
| opacity 0.2s, | |
| filter 0.2s; | |
| } | |
| &:hover limel-badge { | |
| filter: brightness(1.2); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/components/chart/chart.scss around lines 109 to 151 and update usage in
src/components/chart/chart.tsx at line 277: replace the BEM-style modifier class
.legend-item--hidden with a simpler modifier name (e.g., .legend-item-hidden or
.hidden) in the SCSS and update the JSX to add/remove that new class name
instead of .legend-item--hidden so class names match; keep all other styles
intact and ensure hover/transition rules still target the renamed selector.
| private renderLegend(items: ChartItem[]) { | ||
| // Only render legend if at least one item has a color | ||
| const hasColoredItems = items.some((item) => item.color); | ||
|
|
||
| if (!hasColoredItems) { | ||
| return; | ||
| } | ||
|
|
||
| return ( | ||
| <div class="legend"> | ||
| <div class="legend-content"> | ||
| {items.map((item) => { | ||
| const isHidden = this.hiddenItems.has(item.text); | ||
| return ( | ||
| <div | ||
| class={{ | ||
| 'legend-item': true, | ||
| 'legend-item--hidden': isHidden, | ||
| }} | ||
| key={item.text} | ||
| onClick={() => this.handleLegendClick(item)} | ||
| > | ||
| <limel-badge | ||
| style={{ | ||
| '--badge-background-color': item.color, | ||
| cursor: 'pointer', | ||
| opacity: isHidden ? '0.6' : '1', | ||
| }} | ||
| /> | ||
| <span | ||
| style={{ opacity: isHidden ? '0.6' : '1' }} | ||
| > | ||
| {this.getItemText(item)} | ||
| </span> | ||
| </div> | ||
| ); | ||
| })} | ||
| </div> | ||
| </div> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Ensure unique keys for legend items.
Using item.text as the key assumes all items have unique text values. If duplicate text values exist, this will cause key collisions and rendering issues.
Consider one of these solutions:
Solution 1: Use text with index for uniqueness
- {items.map((item) => {
+ {items.map((item, index) => {
const isHidden = this.hiddenItems.has(item.text);
return (
<div
class={{
'legend-item': true,
'legend-item--hidden': isHidden,
}}
- key={item.text}
+ key={`${item.text}-${index}`}
onClick={() => this.handleLegendClick(item)}
>Solution 2: Add a unique ID to ChartItem
If items should have globally unique identifiers, consider adding an optional id field to the ChartItem interface and using that as the key.
📝 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 renderLegend(items: ChartItem[]) { | |
| // Only render legend if at least one item has a color | |
| const hasColoredItems = items.some((item) => item.color); | |
| if (!hasColoredItems) { | |
| return; | |
| } | |
| return ( | |
| <div class="legend"> | |
| <div class="legend-content"> | |
| {items.map((item) => { | |
| const isHidden = this.hiddenItems.has(item.text); | |
| return ( | |
| <div | |
| class={{ | |
| 'legend-item': true, | |
| 'legend-item--hidden': isHidden, | |
| }} | |
| key={item.text} | |
| onClick={() => this.handleLegendClick(item)} | |
| > | |
| <limel-badge | |
| style={{ | |
| '--badge-background-color': item.color, | |
| cursor: 'pointer', | |
| opacity: isHidden ? '0.6' : '1', | |
| }} | |
| /> | |
| <span | |
| style={{ opacity: isHidden ? '0.6' : '1' }} | |
| > | |
| {this.getItemText(item)} | |
| </span> | |
| </div> | |
| ); | |
| })} | |
| </div> | |
| </div> | |
| ); | |
| } | |
| private renderLegend(items: ChartItem[]) { | |
| // Only render legend if at least one item has a color | |
| const hasColoredItems = items.some((item) => item.color); | |
| if (!hasColoredItems) { | |
| return; | |
| } | |
| return ( | |
| <div class="legend"> | |
| <div class="legend-content"> | |
| {items.map((item, index) => { | |
| const isHidden = this.hiddenItems.has(item.text); | |
| return ( | |
| <div | |
| class={{ | |
| 'legend-item': true, | |
| 'legend-item--hidden': isHidden, | |
| }} | |
| key={`${item.text}-${index}`} | |
| onClick={() => this.handleLegendClick(item)} | |
| > | |
| <limel-badge | |
| style={{ | |
| '--badge-background-color': item.color, | |
| cursor: 'pointer', | |
| opacity: isHidden ? '0.6' : '1', | |
| }} | |
| /> | |
| <span | |
| style={{ opacity: isHidden ? '0.6' : '1' }} | |
| > | |
| {this.getItemText(item)} | |
| </span> | |
| </div> | |
| ); | |
| })} | |
| </div> | |
| </div> | |
| ); | |
| } |
🧰 Tools
🪛 Biome (2.1.2)
[error] 274-281: Static Elements should not be interactive.
To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value.
(lint/a11y/noStaticElementInteractions)
[error] 274-281: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
🤖 Prompt for AI Agents
In src/components/chart/chart.tsx around lines 260 to 300, the legend uses
item.text as the React/vdom key which can collide when texts are duplicated; add
a unique key strategy: extend ChartItem with an optional id field (if
appropriate) and use that as the key when present, otherwise fall back to a
deterministic fallback such as combining item.text with the item index (e.g.
`${item.text}-${index}`) to guarantee uniqueness; update the ChartItem
type/interface and the renderLegend mapping to use the new id-or-text-with-index
key.
| <div | ||
| class={{ | ||
| 'legend-item': true, | ||
| 'legend-item--hidden': isHidden, | ||
| }} | ||
| key={item.text} | ||
| onClick={() => this.handleLegendClick(item)} | ||
| > |
There was a problem hiding this comment.
Add keyboard support to legend items for accessibility.
The legend item div has an onClick handler but lacks keyboard event handlers, making it inaccessible to keyboard-only users.
Apply this diff to add keyboard support:
<div
class={{
'legend-item': true,
'legend-item--hidden': isHidden,
}}
key={item.text}
+ tabIndex={0}
+ role="button"
onClick={() => this.handleLegendClick(item)}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ this.handleLegendClick(item);
+ }
+ }}
>📝 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.
| <div | |
| class={{ | |
| 'legend-item': true, | |
| 'legend-item--hidden': isHidden, | |
| }} | |
| key={item.text} | |
| onClick={() => this.handleLegendClick(item)} | |
| > | |
| <div | |
| class={{ | |
| 'legend-item': true, | |
| 'legend-item--hidden': isHidden, | |
| }} | |
| key={item.text} | |
| tabIndex={0} | |
| role="button" | |
| onClick={() => this.handleLegendClick(item)} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| e.preventDefault(); | |
| this.handleLegendClick(item); | |
| } | |
| }} | |
| > |
🧰 Tools
🪛 Biome (2.1.2)
[error] 274-281: Static Elements should not be interactive.
To add interactivity such as a mouse or key event listener to a static element, give the element an appropriate role value.
(lint/a11y/noStaticElementInteractions)
[error] 274-281: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
🤖 Prompt for AI Agents
In src/components/chart/chart.tsx around lines 274 to 281, the legend item div
has an onClick but no keyboard support; make it accessible by adding keyboard
handlers and focusability: add role="button" and tabIndex={0} to make it
focusable/semantically interactive, implement an onKeyDown that listens for
Enter and Space and calls this.handleLegendClick(item) (prevent default on
Space), and include an appropriate aria attribute like aria-pressed or
aria-checked if the item represents toggle state; keep the existing onClick to
call the same handler so mouse and keyboard share behavior.
fix: #3723
Summary by CodeRabbit
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: