feat(chart): add option to display column titles for landscape bar ch… #3742
Conversation
📝 WalkthroughWalkthroughThe changes introduce a column titles feature for landscape-oriented bar charts in the Chart component. A new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
Documentation has been published to https://lundalogik.github.io/lime-elements/versions/PR-3742/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/chart/chart.tsx (1)
333-366: Avoid returning an array fromrenderItemLabel(and fix the key warning)
renderItemLabelcurrently returns an array of JSX elements whenshowColumnTitlesis enabled; this is what triggers the “missing key” warnings from static analysis. In Stencil, it’s preferable to avoid array literals here rather than adding React‑stylekeyprops.You can return a single wrapper element containing both the column title and the tooltip instead:
const tooltip = ( <limel-tooltip {...tooltipProps} openDirection={ this.orientation === 'portrait' ? 'right' : 'top' } /> ); - if ( - this.showColumnTitles && - this.orientation === 'landscape' && - this.type === 'bar' - ) { - return [<div class="column-title">{text}</div>, tooltip]; - } - - return tooltip; + const showColumnTitle = + this.showColumnTitles && + this.orientation === 'landscape' && + this.type === 'bar'; + + if (showColumnTitle) { + return ( + <div> + <div class="column-title">{text}</div> + {tooltip} + </div> + ); + } + + return tooltip;This keeps the DOM structure simple, removes the array literal, and should satisfy the lint checks without introducing
keyprops.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
src/components/chart/chart.scss(1 hunks)src/components/chart/chart.tsx(5 hunks)src/components/chart/examples/chart-column-titles.tsx(1 hunks)src/components/chart/examples/chart-type-bar.tsx(1 hunks)src/components/chart/partial-styles/_bar-gantt-dot.scss(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
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/partial-styles/_bar-gantt-dot.scsssrc/components/chart/chart.scss
**/*.{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/partial-styles/_bar-gantt-dot.scsssrc/components/chart/examples/chart-type-bar.tsxsrc/components/chart/chart.scsssrc/components/chart/examples/chart-column-titles.tsxsrc/components/chart/chart.tsx
**/*.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/examples/chart-type-bar.tsxsrc/components/chart/examples/chart-column-titles.tsxsrc/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/examples/chart-type-bar.tsxsrc/components/chart/examples/chart-column-titles.tsxsrc/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/examples/chart-type-bar.tsxsrc/components/chart/examples/chart-column-titles.tsxsrc/components/chart/chart.tsx
src/components/**/examples/**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
src/components/**/examples/**/*.{ts,tsx}: These files are an exception to the rule that all imports should use relative paths. When these example files import something that is publicly exported by lime-elements, the import should be made from@limetech/lime-elements. If they import something from another file inside theexamplefolder, the import should use a relative path.
Files:
src/components/chart/examples/chart-type-bar.tsxsrc/components/chart/examples/chart-column-titles.tsx
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/examples/chart-type-bar.tsxsrc/components/chart/examples/chart-column-titles.tsxsrc/components/chart/chart.tsx
🧠 Learnings (2)
📚 Learning: 2025-04-25T13:32:56.396Z
Learnt from: adrianschmidt
Repo: Lundalogik/lime-elements PR: 3529
File: src/components/lime-ai-avatar/lime-ai-avatar.tsx:30-38
Timestamp: 2025-04-25T13:32:56.396Z
Learning: For StencilJS components in lime-elements, prefer using the `<Host>` element to wrap child elements in the render method instead of returning an array of elements, which eliminates the need for key attributes and follows StencilJS best practices.
Applied to files:
src/components/chart/examples/chart-column-titles.tsx
📚 Learning: 2025-03-04T14:27:29.714Z
Learnt from: adrianschmidt
Repo: Lundalogik/lime-elements PR: 3464
File: src/components/text-editor/prosemirror-adapter/plugins/image/inserter.ts:1-11
Timestamp: 2025-03-04T14:27:29.714Z
Learning: In example components (typically found in `src/components/*/examples/`), imports of lime-elements exports should use `limetech/lime-elements` instead of relative paths. This is because example components aren't included in type declarations and are used in documentation to show developers how to properly use lime-elements components.
Applied to files:
src/components/chart/chart.tsx
🧬 Code graph analysis (1)
src/components/chart/chart.tsx (1)
src/components/chart/chart.types.ts (1)
ChartItem(7-38)
🪛 Biome (2.1.2)
src/components/chart/chart.tsx
[error] 362-362: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🪛 GitHub Check: SonarCloud Code Analysis
src/components/chart/examples/chart-column-titles.tsx
[warning] 46-46: Member 'handleToggleChange' is never reassigned; mark it as readonly.
src/components/chart/chart.tsx
[warning] 362-362: Missing "key" prop for element in array
⏰ 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/examples/chart-type-bar.tsx (1)
48-54: Correct wiring ofshowColumnTitlesin bar chart exampleUsing the new
showColumnTitlesprop here is consistent with the core chart API and works correctly with the orientation state.src/components/chart/chart.scss (1)
62-67: Scoped styling for.column-titlelooks goodTruncation and absolute positioning are minimal and appropriate, and the class name follows the non‑BEM guideline for shadow‑DOM components.
src/components/chart/partial-styles/_bar-gantt-dot.scss (1)
7-14:position: relativeon.itemis appropriateThis establishes a consistent positioning context for both the new
.column-titleand existing absolute pseudo‑elements on dot charts, without conflicting with the surrounding layout rules.src/components/chart/chart.tsx (2)
15-36: New example reference keeps documentation in syncAdding
@exampleComponent limel-example-chart-column-titlescorrectly exposes the new example in the chart docs.
221-249: Item label rendering hook is correctly integratedSwitching the row rendering to use
renderItemLabelinstead of an inline tooltip keeps the per‑item markup centralized and makes it easier to evolve the label/tooltip behavior.src/components/chart/examples/chart-column-titles.tsx (1)
4-44: Example cleanly demonstratesshowColumnTitlesbehaviorThe example correctly fixes orientation to landscape, wires
showColumnTitlesto local state, and uses<Host>as the render root, which matches the Stencil guidelines.
| /** | ||
| * When set to `true`, displays column titles for bar charts. | ||
| * By default, column titles are not rendered. | ||
| */ | ||
| @Prop({ reflect: true }) | ||
| public showColumnTitles: boolean = false; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Clarify that showColumnTitles applies to landscape bar charts
The implementation only shows column titles when type === 'bar' and orientation === 'landscape', but the doc comment currently mentions only “bar charts”. Consider making that explicit so consumers understand why nothing changes for portrait bar charts.
For example:
/**
* When set to `true`, displays column titles for bar charts
* in landscape orientation. By default, column titles are not rendered.
*/
@Prop({ reflect: true })
public showColumnTitles: boolean = false;🤖 Prompt for AI Agents
In src/components/chart/chart.tsx around lines 115 to 120, the Prop doc comment
for showColumnTitles is misleading because it implies it applies to all bar
charts but the implementation only renders column titles when type === 'bar' and
orientation === 'landscape'; update the JSDoc to explicitly state that the flag
controls column titles for bar charts in landscape orientation (and that
portrait bar charts are unaffected) so consumers understand the scope of the
prop.
There was a problem hiding this comment.
This is actually a red flag. Let's think a bit more about how we should solve this 🤔
- We shouldn't have a prop that only sometimes works. Preferably, we should cover most of the types if possible. Can we somehow display it for others too?
- If this is only for bar chart, then
- it should work for both landscape and portrait modes.
- the name of the prop should be more descriptive
| private handleToggleChange = (event: CustomEvent<boolean>) => { | ||
| this.showColumnTitles = event.detail; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Mark handleToggleChange as readonly
The member is never reassigned and is used as an event handler; marking it readonly matches the pattern used in chart.tsx and satisfies the static analysis suggestion.
-export class ChartColumnTitlesExample {
+export class ChartColumnTitlesExample {
@@
- private handleToggleChange = (event: CustomEvent<boolean>) => {
+ private readonly handleToggleChange = (event: CustomEvent<boolean>) => {
this.showColumnTitles = event.detail;
};📝 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 handleToggleChange = (event: CustomEvent<boolean>) => { | |
| this.showColumnTitles = event.detail; | |
| }; | |
| private readonly handleToggleChange = (event: CustomEvent<boolean>) => { | |
| this.showColumnTitles = event.detail; | |
| }; |
🧰 Tools
🪛 GitHub Check: SonarCloud Code Analysis
[warning] 46-46: Member 'handleToggleChange' is never reassigned; mark it as readonly.
🤖 Prompt for AI Agents
In src/components/chart/examples/chart-column-titles.tsx around lines 46 to 48,
the event handler handleToggleChange is never reassigned but is defined as a
mutable class field; change its declaration to a readonly class field (e.g. add
the readonly modifier to "private readonly handleToggleChange = ...") so it
matches the pattern used in chart.tsx and satisfies the static analysis
suggestion while keeping the same arrow-function/event-handler behavior.
| /** | ||
| * Label for the horizontal axis (X-axis). | ||
| */ | ||
| @Prop({ reflect: true }) | ||
| public xAxisLabel?: string; |
There was a problem hiding this comment.
This one is a new separate feature. It should not be committed together with the other one 😊
It also has the same problem like the previous one. The chart component has various modes or types. Since all of them are 2D, the x-axis is applicable to all. Basically, all of them have an x-axis. But in some, like the circular ones, the meaning of x-axis would be really strange.
Further down, the this prop is implemented in a way where it will always show up. Even under a pie or doughnut chart.
Additionally, there is already a prop called accessibleItemsLabel which is basically what you are trying to do.
/**
* Helps users of assistive technologies to understand
* what the items in the chart represent.
*/
@Prop({ reflect: true })
public accessibleItemsLabel?: string;Further,
On landscape orientation, the x-axis is below. But on it can be on the left side on the portrait mode. So this name becomes confusing
…arts
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.
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: