-
Notifications
You must be signed in to change notification settings - Fork 17
feat(chart): add option to display column titles for landscape bar ch… #3742
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ | |
| * @exampleComponent limel-example-chart-orientation | ||
| * @exampleComponent limel-example-chart-max-value | ||
| * @exampleComponent limel-example-chart-type-bar | ||
| * @exampleComponent limel-example-chart-column-titles | ||
| * @exampleComponent limel-example-chart-type-dot | ||
| * @exampleComponent limel-example-chart-type-area | ||
| * @exampleComponent limel-example-chart-type-line | ||
|
|
@@ -111,6 +112,19 @@ | |
| @Prop({ reflect: true }) | ||
| public loading: boolean = false; | ||
|
|
||
| /** | ||
| * When set to `true`, displays column titles for bar charts. | ||
| * By default, column titles are not rendered. | ||
| */ | ||
| @Prop({ reflect: true }) | ||
| public showColumnTitles: boolean = false; | ||
|
|
||
| /** | ||
| * Label for the horizontal axis (X-axis). | ||
| */ | ||
| @Prop({ reflect: true }) | ||
| public xAxisLabel?: string; | ||
|
Comment on lines
+122
to
+126
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 /**
* Helps users of assistive technologies to understand
* what the items in the chart represent.
*/
@Prop({ reflect: true })
public accessibleItemsLabel?: string;Further, |
||
|
|
||
| private range: { | ||
| minValue: number; | ||
| maxValue: number; | ||
|
|
@@ -132,21 +146,30 @@ | |
| return <limel-spinner limeBranded={false} />; | ||
| } | ||
|
|
||
| return ( | ||
| return [ | ||
| <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> | ||
| ); | ||
| </table>, | ||
|
Check warning on line 162 in src/components/chart/chart.tsx
|
||
| this.renderXAxisLabel(), | ||
| ]; | ||
| } | ||
|
|
||
| private renderXAxisLabel() { | ||
| if (!this.xAxisLabel) { | ||
| return; | ||
| } | ||
|
|
||
| return <div>{this.xAxisLabel}</div>; | ||
| } | ||
|
|
||
| private renderCaption() { | ||
|
|
@@ -237,7 +260,7 @@ | |
| > | ||
| <th>{this.getItemText(item)}</th> | ||
| <td>{this.getFormattedValue(item)}</td> | ||
| {this.renderTooltip(item, itemId, size)} | ||
| {this.renderItemLabel(item, itemId, size)} | ||
| </tr> | ||
| ); | ||
| }); | ||
|
|
@@ -322,7 +345,7 @@ | |
| return item.text; | ||
| } | ||
|
|
||
| private renderTooltip(item: ChartItem, itemId: string, size: number) { | ||
| private renderItemLabel(item: ChartItem, itemId: string, size: number) { | ||
| const text = this.getItemText(item); | ||
| const PERCENT_DECIMAL = 2; | ||
| const formattedValue = this.getFormattedValue(item); | ||
|
|
@@ -337,14 +360,24 @@ | |
| tooltipProps.label = `${text} (${size.toFixed(PERCENT_DECIMAL)}%)`; | ||
| } | ||
|
|
||
| return ( | ||
| 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]; | ||
|
Check warning on line 377 in src/components/chart/chart.tsx
|
||
| } | ||
|
|
||
| return tooltip; | ||
| } | ||
|
|
||
| private calculateRange() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,50 @@ | ||||||||||||||
| import { Component, h, Host, State } from '@stencil/core'; | ||||||||||||||
| import { chartItems } from './chart-items-bar'; | ||||||||||||||
|
|
||||||||||||||
| /** | ||||||||||||||
| * Bar chart with column titles | ||||||||||||||
| * | ||||||||||||||
| * By setting the `showColumnTitles` prop to `true`, you can display column titles | ||||||||||||||
| * for bar charts in landscape orientation. This is useful when you want to clearly | ||||||||||||||
| * label each bar with its category name directly in the chart, rather than relying | ||||||||||||||
| * on tooltips. | ||||||||||||||
| * | ||||||||||||||
| * When `showColumnTitles` is `false` (the default), the chart will use tooltips | ||||||||||||||
| * to display item information on hover instead. | ||||||||||||||
| */ | ||||||||||||||
| @Component({ | ||||||||||||||
| tag: 'limel-example-chart-column-titles', | ||||||||||||||
| shadow: true, | ||||||||||||||
| styleUrl: 'chart-examples.scss', | ||||||||||||||
| }) | ||||||||||||||
| export class ChartColumnTitlesExample { | ||||||||||||||
| @State() | ||||||||||||||
| private showColumnTitles = true; | ||||||||||||||
|
|
||||||||||||||
| public render() { | ||||||||||||||
| return ( | ||||||||||||||
| <Host class="large"> | ||||||||||||||
| <h4>Monthly Sales Performance</h4> | ||||||||||||||
| <limel-chart | ||||||||||||||
| type="bar" | ||||||||||||||
| items={chartItems} | ||||||||||||||
| orientation="landscape" | ||||||||||||||
| showColumnTitles={this.showColumnTitles} | ||||||||||||||
| xAxisLabel="Months" | ||||||||||||||
| maxValue={100} | ||||||||||||||
| /> | ||||||||||||||
| <limel-example-controls> | ||||||||||||||
| <limel-switch | ||||||||||||||
| label="Show column titles" | ||||||||||||||
| value={this.showColumnTitles} | ||||||||||||||
| onChange={this.handleToggleChange} | ||||||||||||||
| /> | ||||||||||||||
| </limel-example-controls> | ||||||||||||||
| </Host> | ||||||||||||||
| ); | ||||||||||||||
| } | ||||||||||||||
|
|
||||||||||||||
| private handleToggleChange = (event: CustomEvent<boolean>) => { | ||||||||||||||
|
Check warning on line 47 in src/components/chart/examples/chart-column-titles.tsx
|
||||||||||||||
| this.showColumnTitles = event.detail; | ||||||||||||||
| }; | ||||||||||||||
|
Comment on lines
+47
to
+49
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick | 🔵 Trivial Mark The member is never reassigned and is used as an event handler; marking it -export class ChartColumnTitlesExample {
+export class ChartColumnTitlesExample {
@@
- private handleToggleChange = (event: CustomEvent<boolean>) => {
+ private readonly handleToggleChange = (event: CustomEvent<boolean>) => {
this.showColumnTitles = event.detail;
};📝 Committable suggestion
Suggested change
🧰 Tools🪛 GitHub Check: SonarCloud Code Analysis[warning] 46-46: Member 'handleToggleChange' is never reassigned; mark it as 🤖 Prompt for AI Agents |
||||||||||||||
| } | ||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,7 @@ | |
| } | ||
|
|
||
| .item { | ||
| position: relative; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion | 🟠 Major
Clarify that
showColumnTitlesapplies to landscape bar chartsThe implementation only shows column titles when
type === 'bar'andorientation === '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:
🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually a red flag. Let's think a bit more about how we should solve this 🤔