-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Introduce a new annotated version of Horizontal Bar Charts #34825
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: master
Are you sure you want to change the base?
Introduce a new annotated version of Horizontal Bar Charts #34825
Conversation
...es/charts/react-charts/library/src/components/HorizontalBarChart/HorizontalBarChart.types.ts
Show resolved
Hide resolved
...es/charts/react-charts/library/src/components/HorizontalBarChart/HorizontalBarChart.types.ts
Outdated
Show resolved
Hide resolved
packages/charts/react-charts/library/src/components/HorizontalBarChart/HorizontalBarChart.tsx
Outdated
Show resolved
Hide resolved
packages/charts/react-charts/library/src/components/HorizontalBarChart/HorizontalBarChart.tsx
Outdated
Show resolved
Hide resolved
|
||
let prevPosition = 0; | ||
|
||
const annotatedBars = adjustedSegments!.map((segment, index) => { |
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.
what happens if there are 19 bars with value of 1 and 1 bar with value of 100. How does the chart look
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.
Please refer to the last bar in the screenshot for reference.
Our designer has specifically requested that each segment must have a minimum width, even if its value is very small, so that there's enough space to display the annotation.
As a result, larger segments will need to be scaled down, and the visual proportions will no longer accurately reflect the actual data values.
This is a conscious design decision, annotation visibility is being prioritized over exact visual accuracy.

@@ -195,7 +211,7 @@ export interface HorizontalBarChartStyles { | |||
* percentage: show the percentage of (datapoint.x/datapoint.y)% | |||
* {@docCategory HorizontalBarChart} | |||
*/ | |||
export type ChartDataMode = 'default' | 'fraction' | 'percentage'; | |||
export type ChartDataMode = 'default' | 'fraction' | 'percentage' | 'hidden'; |
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.
you are not hiding the labels, but moving it to the bottom. Can you adjust the mode accordingly.
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.
Context:
Currently, each bar in the chart displays the total (sum of all its segments) at the top-right corner. However, our design explicitly does not want this total to be shown. This is not the same as per-segment annotations. The value in question is a summation of all segments in a bar, and we want to completely hide it.

Problem:
There is no existing way to disable this behavior as the sum is always rendered.
Proposed Solution:
I've introduced a new prop and documented it. When this prop is enabled, the chart will return early from the function that renders the bar total, effectively skipping its rendering.
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 value is for the showAnnotationsInPercentage prop that you have added. Instead of adding 1 more prop, you can add a variant to chartDataMode called 'annotationsPercent' and use it to hide or customize the label
@@ -204,6 +210,149 @@ export const HorizontalBarChart: React.FunctionComponent<HorizontalBarChartProps | |||
); | |||
} | |||
|
|||
function _createAnnotatedBars(data: ChartProps, minSegmentMinWidthPercent: number): JSX.Element[] { |
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.
How to avoid the code duplication introduced in this function
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’d prefer not to refactor this function as part of this change. Since it involves both existing logic and the new variant, there’s a risk of mixing logic and unintentionally introducing bugs. I think it’s safer to keep this new variant focused code separate since it is a unique use case and no other team will likely need such a tailored version.
That said, if you feel strongly about refactoring the common code, I’m happy to give it a try, just wanted to flag the potential risk.
@microsoft-github-policy-service agree company="Microsoft" |
a357611
to
2100736
Compare
*/ | ||
allowHoverOnSegment?: boolean; | ||
|
||
// renderAnnotationAddon?: (index: number) => JSX.Element; |
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.
2100736
to
f9626ca
Compare
@@ -204,3 +230,9 @@ export enum HorizontalBarChartVariant { | |||
PartToWhole = 'part-to-whole', | |||
AbsoluteScale = 'absolute-scale', | |||
} | |||
|
|||
export type segment = { |
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 don't see this exported type being used anywhere outside the control
tokens.colorPaletteDarkGreenForeground2, | ||
tokens.colorPaletteNavyForeground2, | ||
tokens.colorPaletteDarkOrangeForeground2, | ||
]; |
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 defaultColors array can be refactored without any issue
@@ -150,7 +150,7 @@ Object { | |||
</g> | |||
</g> | |||
<g | |||
aria-label="Mar/01, p2. Nasty, 50/2391. a good day to start with in Texas with best air quality." | |||
aria-label="Feb/29, p2. Nasty, 50/2391. a good day to start with in Texas with best air quality." |
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.
@@ -204,6 +211,169 @@ export const HorizontalBarChart: React.FunctionComponent<HorizontalBarChartProps | |||
); | |||
} | |||
|
|||
function _createAnnotatedBars(data: ChartProps): JSX.Element[] { |
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.
📊 Bundle size report✅ No changes found |
Pull request demo site: URL |
@@ -0,0 +1,7 @@ | |||
{ |
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.
🕵🏾♀️ visual changes to review in the Visual Change Report
vr-tests-react-components/Charts-DonutChart 1 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
vr-tests-react-components/Charts-DonutChart.Dynamic - RTL.default.chromium.png | 30793 | Changed |
vr-tests-react-components/Drawer 1 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
vr-tests-react-components/Drawer.overlay drawer full - High Contrast.chromium.png | 6758 | Changed |
vr-tests-react-components/Positioning 2 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
vr-tests-react-components/Positioning.Positioning end.chromium.png | 844 | Changed |
vr-tests-react-components/Positioning.Positioning end.updated 2 times.chromium.png | 314 | Changed |
vr-tests-react-components/Skeleton converged 1 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
vr-tests-react-components/Skeleton converged.Opaque Skeleton with circle - Dark Mode.default.chromium.png | 3 | Changed |
vr-tests-react-components/TagPicker 2 screenshots
Image Name | Diff(in Pixels) | Image Type |
---|---|---|
vr-tests-react-components/TagPicker.disabled - Dark Mode.disabled input hover.chromium.png | 659 | Changed |
vr-tests-react-components/TagPicker.disabled - High Contrast.disabled input hover.chromium.png | 1321 | Changed |
There were 2 duplicate changes discarded. Check the build logs for more information.
Previous Behavior
We did not have a variant for the Horizontal Bar chart that would show the percentages for the segments under each.
New Behavior
Adding a version of the horizontal bar chart where we can see the percents of each segment under each.
As per our designer, each segment shall have a minimum width which will not go below 35px. This is done to accommodate the annotation and an optional JSX element that may be rendered beside the annotation below each segment.
There is an edge case where 30px is not available, in that case, each segment will still have a minimum width but may be smaller than 35px. And in such cases, we may not show the annotation text at all and only the optional JSX element.
On small screens
Related Issue(s)