-
Notifications
You must be signed in to change notification settings - Fork 71
LG-5588 more chart customization points #3274
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
Conversation
🦋 Changeset detectedLatest commit: 29f71a9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Size Change: +1.99 kB (+0.11%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
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.
Pull Request Overview
This PR adds customization options to the charts library to support different chart visualization needs, particularly for bar charts. The changes introduce three new configuration points: tooltip axis pointer control, bar hover emphasis behavior, and a category axis type for discrete data alignment.
Key changes:
- Added
axisPointerprop toChartTooltipto control the vertical hover line visibility - Added
emphasisprop toBarcomponent for controlling hover focus behavior - Added
categorytype toXAxisfor proper discrete data label alignment
Reviewed Changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
charts/core/src/XAxis/XAxis.types.ts |
Adds Category axis type with documentation explaining its use for discrete data |
charts/core/src/Series/Bar/Bar.tsx |
Adds emphasis prop and refactors component to use destructured props |
charts/core/src/ChartTooltip/ChartTooltip.types.ts |
Adds axisPointer prop type definition with documentation |
charts/core/src/ChartTooltip/ChartTooltip.tsx |
Implements axisPointer configuration and adds dark mode fix class |
charts/core/src/Chart.stories.tsx |
Adds three new stories demonstrating the new customization features |
.changeset/rotten-parrots-stare.md |
Documents the changes for release notes |
charts/core/src/Series/Bar/Bar.tsx
Outdated
| * - `self`: Upon hovering over a bar, all other bars will be dimmed. | ||
| * - `none`: Other bars will not be affected by the hover. | ||
| */ | ||
| emphasis?: 'self'; |
Copilot
AI
Oct 31, 2025
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.
The emphasis prop only accepts 'self' but the documentation mentions 'none' as an option. Consider adding 'none' to the type definition: emphasis?: 'self' | 'none' to match the documented behavior and the story's argTypes.
| emphasis?: 'self'; | |
| emphasis?: 'self' | 'none'; |
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.
Fixed! ✨ The emphasis prop now includes both 'self' | 'none' in the type definition (commit b3fdac1).
| * Controls the type of axis pointer shown with the tooltip. Default is 'line' | ||
| * which shows a vertical dashed line on hover. Set to 'none' for bar charts to | ||
| * avoid the dashed line from being rendered on top of the bars. |
Copilot
AI
Oct 31, 2025
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.
Consider defining a default value in the type documentation. The comment states 'Default is 'line'' but the type doesn't enforce this. Either add = 'line' in the component signature or explicitly handle the undefined case in the implementation.
| * Controls the type of axis pointer shown with the tooltip. Default is 'line' | |
| * which shows a vertical dashed line on hover. Set to 'none' for bar charts to | |
| * avoid the dashed line from being rendered on top of the bars. | |
| * Controls the type of axis pointer shown with the tooltip. | |
| * If not provided, the default is 'line', which shows a vertical dashed line on hover. | |
| * Set to 'none' for bar charts to avoid the dashed line from being rendered on top of the bars. |
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.
The axisPointer parameter now has a default value of 'line' in the component signature (ChartTooltip.tsx line 21), so the behavior matches the documentation 👍
tsck
left a comment
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.
A few thoughts but otherwise looks good!
| * with the tooltip styles. | ||
| */ | ||
| className: CHART_TOOLTIP_CLASSNAME, | ||
| className: `${CHART_TOOLTIP_CLASSNAME} darkreader-ignore-inline-style`, |
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.
I'm wondering if this should be built into the component or if we should allow the component to accept a className prop, that way it can be passed in if needed. My thought is that this is MMS specific. What do you think?
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.
Correct, and good point! I changed this as you advised to be more flexible. ChartTooltip component now accepts an optional className prop (commit b3fdac1). MMS can now pass in the class darkreader-ignore-inline-style to avoid dark mode issues.
charts/core/src/XAxis/XAxis.types.ts
Outdated
| * using 'category', the charting library may automatically determine axis label positions, | ||
| * which might not correspond to each bar or data point. | ||
| */ | ||
| Category: 'category', |
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 was originally included but removed before v1 since it made no sense for line charts. Makes sense though for a bar chart of course! If we're going to add this to the x-axis, should we add it to the y-axis as well? It might not add much value but perhaps for consistency it makes sense.
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.
Great call on keeping things consistent! I also discovered with more testing that for the 'category' axis type, it's important to always provide the data prop. otherwise, Apache ECharts won't display the category labels correctly.
To improve consistency and avoid code duplication, I factored out all common axis properties and types into a single shared Axis.types.ts file. Now, both XAxisProps and YAxisProps extend a common AxisProps type, which supports the 'category' type.
charts/core/src/Series/Bar/Bar.tsx
Outdated
| * - `self`: Upon hovering over a bar, all other bars will be dimmed. | ||
| * - `none`: Other bars will not be affected by the hover. | ||
| */ | ||
| emphasis?: 'self'; |
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.
Agree with copilot here, I think you missed 'none' in the type here.
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.
Fixed! Added 'none' to the type definition as you and Copilot both suggested (commit b3fdac1) 🎉
a0283de to
7134bf4
Compare
|
Hi @adamrasheed, I removed Terrence from reviewers as I noticed he's on vacation until Nov 10 and you were auto-assigned. Kindly please take a look whenever you had time 🙏 |
charts/core/src/Axis/Axis.types.ts
Outdated
| */ | ||
| label?: string; | ||
| } & ( | ||
| | { |
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.
nit: not necessary, but it might be cleaner to create ContinuousAxisProps and DiscreteAxisProps, and then set AxisProps = ContinuousAxisProps | DiscreteAxisProps
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.
Especially now that I see we export ContinuousAxisProps anyway
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.
Like your suggestion! 💡 Refactor it to:
type AxisPropsBase = {...}
export type ContinuousAxisProps = {...}
export type DiscreteAxisProps = {...}
export type AxisProps = ContinuousAxisProps | DiscreteAxisProps
charts/core/src/Axis/Axis.ts
Outdated
| ...(type !== 'category' && { min: props.min }), | ||
| ...(type !== 'category' && { max: props.max }), | ||
| ...(type === 'category' && { data: props.data }), |
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.
nit: don't love this, but not sure if any alternative would be cleaner. This will get clunky if we have multiple non-continuous axis types
charts/core/src/Series/Bar/Bar.tsx
Outdated
| * - `self`: Upon hovering over a bar, all other bars will be dimmed. | ||
| * - `none`: Other bars will not be affected by the hover. | ||
| */ | ||
| emphasis?: 'self' | 'none'; |
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.
I think hoverBehavior (or focusBehavior) would be a clearer prop name here
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.
Sure, done.
One point though, hoverBehavior is definitely more descriptive than emphasis but the only reason I used emphasis is because that's the exact property name in Apache ECharts' API, which makes it easier to map directly to the underlying library.
I'm happy either way, let me know what you think.
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.
One of the principles we had when specing the initial charts work was that we didn't want to tie ourselves too closely to the ECharts library, in case they change things, or we decide to go with a different library, our API is just abstractions of that library
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.
Hmm, I see. I imagine that it'll be a difficult migration as we get more comfortable and tie LG to more features of ECharts. 😁
stephl3
left a comment
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.
hi @nima-taheri-mongodb I'm doing a more comprehensive review now but have 1 ask while continuing to review: can you use git mv for the file subdir changes? this helps with preserving the git history log
charts/core/src/Series/Bar/index.ts
Outdated
| @@ -1,2 +1 @@ | |||
| export type { BarProps } from './Bar'; | |||
| export { Bar } from './Bar'; | |||
| export { Bar, BarHoverBehavior } from './Bar'; | |||
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.
I think this is why the builds are failing
/home/runner/work/leafygreen-ui/leafygreen-ui/charts/core/src/Series/index.ts (1,20): Module '"./Bar"' has no exported member 'BarProps'.
ELIFECYCLE Command failed with exit code 1.
Error: command finished with error: command (/home/runner/work/leafygreen-ui/leafygreen-ui/charts/core) /home/runner/setup-pnpm/node_modules/.bin/pnpm run tsc exited (1)
| export { Bar, BarHoverBehavior } from './Bar'; | |
| export { Bar, BarHoverBehavior, type BarProps } from './Bar'; |
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.
Thanks Adam! I noticed it and fixed it on my laptop.
Currently trying to digest Stephen's suggestion (I'm getting myself familiar with how git persists renames and moves when file content changes) and see if I can preserve git history properly.
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.
I haven't used git mv retroactively, so I'm not sure if it's possible. You likely would need to reset to the commit prior to moving the files, run git mv, then re-apply changes. It's not imperative, so feel free to ignore that feedback if time is sensitive
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.
I appreciated your suggestion about using git mv to help preserve git history. Definitely important!
From what I understand, git actually tracks changes as additions and deletions, and doesn't record file moves explicitly. File move detection is performed by git using heuristics (like checking content similarity or hashes), not by storing the movement as part of the commit history.
You made me question my assumptions especially by mentioning the git mv command so I dug in a bit more. It seems that in cases where both file path and content changes, you generally need to commit the move separately from any edits. Otherwise, git's move detection will often miss it if the file changes significantly at the same time.
Since our github workflow enforces squash-and-merge (combine all PR commits into one), any intermediate move commits would get flattened, and we'd lose that history anyway if there were substantial changes to the file contents (which is the case in my PR). So I don't see any way to preserve the move history in this case.
Let me know if I'm misunderstanding something or there's a way to do it.
stephl3
left a comment
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.
Was able to get this working locally and all looks/works well!
charts/core/src/Series/Bar/index.ts
Outdated
| @@ -1,2 +1 @@ | |||
| export type { BarProps } from './Bar'; | |||
| export { Bar } from './Bar'; | |||
| export { Bar, BarHoverBehavior } from './Bar'; | |||
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.
do we need to keep the BarProps export?
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.
just checked locally, and that seems to be what's breaking the build
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.
Hey Stephen, that's correct.
charts/core/src/index.ts
Outdated
| EventMarkerPoint, | ||
| type EventMarkerPointProps, | ||
| } from './EventMarkers'; | ||
| export { Bar, type BarProps, Line, type LineProps } from './Series'; |
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.
do we want to add BarHoverBehavior as an export for consumers?
charts/core/src/Series/Bar/Bar.tsx
Outdated
|
|
||
| /** | ||
| * Hover focus behavior for the series. | ||
| * - `dim_other_bars`: Upon hovering over a bar, all other bars will be dimmed. |
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.
| * - `dim_other_bars`: Upon hovering over a bar, all other bars will be dimmed. | |
| * - `dim-others`: Upon hovering over a bar, all other bars will be dimmed. |
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.
sorry! 🫡
| * - `dim_other_bars`: Upon hovering over a bar, all other bars will be dimmed. | ||
| * - `none`: Other bars will not be affected by the hover. | ||
| */ | ||
| hoverBehavior?: BarHoverBehavior; |
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.
can we add an explicit default that this defaults to BarHoverBehavior.None so it's clearer for consumers?
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.
that's a very good point, applied.
charts/core/src/Series/Bar/Bar.tsx
Outdated
| function getEmphasisFocus( | ||
| hoverBehavior?: BarHoverBehavior, | ||
| ): 'series' | 'self' | 'none' | undefined { | ||
| switch (hoverBehavior) { | ||
| case undefined: | ||
| return undefined; | ||
| case BarHoverBehavior.DimOthers: | ||
| return 'self'; | ||
| case BarHoverBehavior.None: | ||
| return 'none'; | ||
| } | ||
| } |
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.
does this intentionally never provide 'series' as a value?
also, instead of mapping undefined to undefined, I might use case default: return 'none'; since that is the default that echarts uses https://echarts.apache.org/en/option.html#series-bar.emphasis.focus
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.
It's confusing, I feel you. I shouldn't have added series to the return type.
Specifying return type here seems unnecessary here anyway as compiler can safely infer it, I'll remove it.
The reason why I did not add it to LG is that, I'm onboard with Terrence and Adam's idea of not using all features that EChart provides unless it's demanded by the design team. All changes I made in this PR were asked by the design team, for instance allowing 'none' is what designer has specifically asked for.
| export const AxisPointerType = { | ||
| None: 'none', | ||
| Line: 'line', | ||
| Shadow: 'shadow', | ||
| } as const; |
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.
Is there a reason we exclude 'cross' as an option?
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.
The vision LG folks have for chart-component is to only expose those features that are demanded by the design team. We can add this later when it was needed.
| /** | ||
| * Additional CSS class names to apply to the tooltip element. | ||
| * Useful for applying environment-specific styles like dark mode overrides. | ||
| */ | ||
| className?: string; |
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.
Curious if you can share more about what overrides are required here?
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.
charts/core/src/index.ts
Outdated
| @@ -1,3 +1,4 @@ | |||
| export { XAxis, type XAxisProps, YAxis, type YAxisProps } from './Axis'; | |||
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.
Could export XAxisType and YAxisType here as well
| }, | ||
| }; | ||
|
|
||
| export const BarWithOnHoverDimOthersBehavior: StoryObj<{ |
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.
The snapshot for this story will likely look like a duplicate of that of the Bar story. For this story, we should probably do 1 of the following:
- Make it an interaction test ensuring that the snapshot shows what it looks like when a specific datapoint in a series is hovered. Searching for
userEvent.hoverin**/*.stories.tsxfiles should yield some codebase examples. Here are docs as well: https://storybook.js.org/docs/writing-tests/interaction-testing#simulating-behavior-with-userevent - Disable this snapshot. Searching for
disableSnapshotshould yield some codebase examples
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.
Good call! I was a bit hesitant to write an interaction test for this story because Apache ECharts in its current configuration just draws on a canvas so common ways of finding sub-elements inside it and interacting with them don't work.
But thanks for nudging me to do it, I looked up its official docs and a little bit of LLM help and managed to trigger the highlight effect.
.changeset/rotten-parrots-stare.md
Outdated
| - ChartTooltip: Added `className` prop for custom styling | ||
| - Bar: `emphasis` prop now accepts 'self' and 'none' options to control hover focus behavior | ||
| - Axis Types: Added 'category' axis type for discrete data (e.g., bar charts) with properly aligned labels | ||
| - Bug Fix: Fixed null/undefined check in CustomTooltip to properly handle falsy values like 0 and empty strings No newline at end of file |
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.
I would consolidate this under the ChartTooltip notes since consumers are not aware of CustomTooltip
.changeset/rotten-parrots-stare.md
Outdated
| Chart improvements and refactoring: | ||
| - ChartTooltip: Added `axisPointer` prop supporting 'line', 'shadow', and 'none' options | ||
| - ChartTooltip: Added `className` prop for custom styling | ||
| - Bar: `emphasis` prop now accepts 'self' and 'none' options to control hover focus behavior |
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.
should update to reflect the consumer API changes
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.
🫡
.changeset/rotten-parrots-stare.md
Outdated
| - ChartTooltip: Added `axisPointer` prop supporting 'line', 'shadow', and 'none' options | ||
| - ChartTooltip: Added `className` prop for custom styling | ||
| - Bar: `emphasis` prop now accepts 'self' and 'none' options to control hover focus behavior | ||
| - Axis Types: Added 'category' axis type for discrete data (e.g., bar charts) with properly aligned labels |
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.
can you add explicit details about how the discriminated union props API works for XAxis / YAxis?
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.
fair, will do 🫡
|
Thanks Stephen for the second round of feedback! going to bed now 😂😴 hope I covered everything. I'll re-check everything tomorrow morning when I'm fresh. |
stephl3
left a comment
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.
LGTM
charts/core/src/index.ts
Outdated
| export { Bar, type BarProps, Line, type LineProps } from './Series'; | ||
| export { | ||
| Bar, | ||
| type BarHoverBehavior, |
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.
excluding the type keyword will allow consumers to use this as an enum
| type BarHoverBehavior, | |
| BarHoverBehavior, |
.changeset/rotten-parrots-stare.md
Outdated
| - ChartTooltip: Added `axisPointer` prop supporting 'line', 'shadow', and 'none' options | ||
| - ChartTooltip: Added `className` prop for custom styling | ||
| - Bar: `hoverBehavior` prop now accepts 'dim-others' and 'none' options to control hover focus behavior | ||
| - XAxis/YAxis: Introduced a new `category` axis type for discrete/categorical datasets (such as for X axes in bar charts). | ||
| - It uses a dedicated axis type definition and a `labels` prop for specifying category names. | ||
| - Existing continuous axis types (`'log'`, `'time'`, `'value'`) remain unchanged, continuing to support `min`, `max`, and `formatter` for customization. | ||
| - Bug Fix: ChartTooltip now renders correctly for values like 0 and empty strings by only checking for null or undefined, not all falsy values No newline at end of file |
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.
nit
| - ChartTooltip: Added `axisPointer` prop supporting 'line', 'shadow', and 'none' options | |
| - ChartTooltip: Added `className` prop for custom styling | |
| - Bar: `hoverBehavior` prop now accepts 'dim-others' and 'none' options to control hover focus behavior | |
| - XAxis/YAxis: Introduced a new `category` axis type for discrete/categorical datasets (such as for X axes in bar charts). | |
| - It uses a dedicated axis type definition and a `labels` prop for specifying category names. | |
| - Existing continuous axis types (`'log'`, `'time'`, `'value'`) remain unchanged, continuing to support `min`, `max`, and `formatter` for customization. | |
| - Bug Fix: ChartTooltip now renders correctly for values like 0 and empty strings by only checking for null or undefined, not all falsy values | |
| - ChartTooltip | |
| - Added `axisPointer` prop supporting 'line', 'shadow', and 'none' options | |
| - Added `className` prop for custom styling | |
| - Bug Fix: renders correctly for values like 0 and empty strings by only checking for null or undefined, not all falsy values | |
| - Bar: `hoverBehavior` prop now accepts 'dim-others' and 'none' options to control hover focus behavior | |
| - XAxis/YAxis: Introduced a new `category` axis type for discrete/categorical datasets (such as for X axes in bar charts). | |
| - It uses a dedicated axis type definition and a `labels` prop for specifying category names. | |
| - Existing continuous axis types (`'log'`, `'time'`, `'value'`) remain unchanged, continuing to support `min`, `max`, and `formatter` for customization. |
charts/core/src/Series/index.ts
Outdated
| @@ -1,3 +1,3 @@ | |||
| export { Bar, type BarProps } from './Bar'; | |||
| export { Bar, type BarHoverBehavior, type BarProps } from './Bar'; | |||
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 should fix build
| export { Bar, type BarHoverBehavior, type BarProps } from './Bar'; | |
| export { Bar, BarHoverBehavior, type BarProps } from './Bar'; |
|
Coverage after merging lg_5588_chart_improvements into main will be
Coverage Report
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Thanks @TheSonOfThomp and @stephl3 for your feedback and expediting this PR to be shipped. If there's any extra improvement suggestion you have in mind, let me know and I'll apply in a next PR. So far we listed:
|

🎫 Ticket
LG-5588
📝 Description
This PR introduces several enhancements to the chart components, focusing on providing more customization options and improving the overall developer experience. The core of this update is a significant refactor of the Axis components, which are now more robust and feature-rich.
✨ New Features & Enhancements
XAxisandYAxiscomponents have been refactored into a unifiedAxiscomponent. This ensures that the axis components are more consistent and easier to maintain.ChartTooltipaxisPointerprop: You can now control the visibility and style of the tooltip's axis pointer. The available options are'line'(default, suitable for continuous axes),'shadow'(suitable for bar charts), and'none'.BarhoverBehaviorprop: A new prop to control the hover focus behavior. Use'dim-others'to dim other bars when hovering over one, or'none'to maintain all bars at full opacity.XAxis&YAxiscategorytype: A newcategoryaxis type has been introduced for discrete data, ensuring that labels align correctly under their corresponding bars in bar charts. Uselabelsprop to customize category labels.darkreader-ignore-inline-styleclass to prevent dark mode browser extensions from interfering with their styles.0and empty string values.🧪 Testing
BarWithOnHoverDimOthersBehavior,BarWithCustomAxisPointer, andBarWithCategoryAxisLabel.