Negative Y Values Support and Example story for VBC #33369
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…soft/fluentui into charting/declarativeCharts
…eCharts' of https://github.com/microsoft/fluentui into charting/declarativeCharts
|
Pull request demo site: URL |
|
For this PR target to master |
🕵 fluentui-web-components-v3 No visual regressions between this PR and main |
fabricteam
reviewed
Nov 29, 2024
| @@ -108,6 +108,7 @@ module.exports = { | |||
| 'MultiStackedBarChart', | |||
There was a problem hiding this comment.
🕵🏾♀️ visual regressions to review in the fluentuiv8 Visual Regression Report
Callout 4 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| Callout.Left center.chromium.png | 2616 | Changed |
| Callout.No callout width specified.chromium.png | 2319 | Changed |
| Callout.Left top edge.chromium.png | 1949 | Changed |
| Callout.Top auto edge.chromium.png | 2307 | Changed |
react-charting-AreaChart 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| react-charting-AreaChart.Custom Accessibility.chromium.png | 11 | Changed |
react-charting-VerticalBarChart 1 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| react-charting-VerticalBarChart.Basic - Secondary Y Axis.chromium.png | 4 | Changed |
🕵 FluentUIV0 No visual regressions between this PR and main |
fabricteam
reviewed
Nov 29, 2024
| @@ -108,6 +108,7 @@ module.exports = { | |||
| 'MultiStackedBarChart', | |||
There was a problem hiding this comment.
🕵🏾♀️ visual regressions to review in the fluentuiv9 Visual Regression Report
Avatar Converged 2 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| Avatar Converged.badgeMask.chromium.png | 4 | Changed |
| Avatar Converged.Badge Mask RTL.chromium.png | 5 | Changed |
Drawer 3 screenshots
| Image Name | Diff(in Pixels) | Image Type |
|---|---|---|
| Drawer.Full Overlay RTL.chromium.png | 1171 | Changed |
| Drawer.Full Overlay High Contrast.chromium.png | 11 | Changed |
| Drawer.overlay drawer full.chromium.png | 1150 | Changed |
srmukher
reviewed
Nov 29, 2024
packages/react-examples/src/react-charting/VerticalBarChart/index.stories.tsx
Show resolved
Hide resolved
srmukher
reviewed
Nov 29, 2024
📊 Bundle size report
Unchanged fixtures
|
AtishayMsft
reviewed
Dec 1, 2024
| * DeclarativeChart component. | ||
| * {@docCategory DeclarativeChart} | ||
| */ | ||
| export const DeclarativeChart: React.FunctionComponent<DeclarativeChartProps> = React.forwardRef< |
There was a problem hiding this comment.
This PR should have only negative value changes. And not declarative chart changes.
There was a problem hiding this comment.
Okay. Closing this PR.
AtishayMsft
reviewed
Dec 1, 2024
| if (barHeight <= 0) { | ||
| const barHeight: number = this.props.supportNegativeYValues | ||
| ? yBarScale(point.y) - yBarScale(0) | ||
| : yBarScale(point.y); |
There was a problem hiding this comment.
Math.max function was necessary before because the chart didn't handle negative values. When negative values were encountered, it would force the bar height to 0, ensuring that no bars went below the baseline.
But now new logic explicitly handles the negative values by adjusting the scale calculation.
AtishayMsft
reviewed
Dec 1, 2024
| const minBarHeight = Math.ceil(yBarScale(this._yMax - this._yMin) / 100.0); | ||
| let adjustedBarHeight = barHeight; | ||
|
|
||
| if (Math.abs(barHeight) === 0 || (barHeight < 0 && !this.props.supportNegativeYValues)) { |
There was a problem hiding this comment.
barHeight <= 0 should be a sufficient condition.
There was a problem hiding this comment.
using barHeight<=0 will also lead to negative height bars to not render. But now we don't want that. We will now not render anything for barHeight =0 or if (negative value prop is false and bar height is negative).
AtishayMsft
reviewed
Dec 1, 2024
| @@ -347,7 +347,13 @@ export function prepareDatapoints( | |||
| : (maxVal - minVal) / splitInto >= 1 | |||
| ? Math.ceil((maxVal - minVal) / splitInto) | |||
| : (maxVal - minVal) / splitInto; | |||
| const dataPointsArray: number[] = [minVal, minVal + val]; | |||
| const dataPointsArray: number[] = [minVal < 0 && maxVal >= 0 ? 0 : minVal]; | |||
| if (minVal < 0 && maxVal >= 0) { | |||
There was a problem hiding this comment.
what happens if both minVal and maxVal < 0
There was a problem hiding this comment.
For (minVal<0 and maxVal<0) and (minVal>0 and maxVal>0)
the default while loop will run
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related Issue(s)