Conversation
🟡 Heimdall Review Status
🟡
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
🟡
0/1
|
Denominator calculation
|
| /** | ||
| * Whether to animate gradient changes. | ||
| */ | ||
| animate?: boolean; |
There was a problem hiding this comment.
Moving this to base props
| ); | ||
| })} | ||
| </linearGradient> | ||
| </motion.linearGradient> |
There was a problem hiding this comment.
Support animations on gradient
| }, | ||
| { | ||
| price: '0.06', | ||
| price: '1173.74', |
There was a problem hiding this comment.
Fixing a random bug I've been seeing with price
| * Baseline value for the area. | ||
| * When set, overrides the default baseline. | ||
| */ | ||
| areaBaseline?: number; |
There was a problem hiding this comment.
Baseline on Areas were broken so having this at the axis level should solve it.
|
|
||
| return <LinearGradient colors={colors} end={end} positions={positions} start={start} />; | ||
| }); | ||
| return <LinearGradient colors={colors} end={end} positions={positions} start={start} />; |
There was a problem hiding this comment.
Enabling animations on mobile was much harder, this follows Path.tsx logic
| stackGap: number, | ||
| layout: CartesianChartLayout, | ||
| baseline: number, | ||
| baselinePx: number, |
There was a problem hiding this comment.
There was a lot of confusion in the code about what baseline was before, we need both baseline and baselinePx here.
We base rounding and gaps based on which side of the baseline a value is (and whether or not it is touching baseline) but we also need the pixel coordinate of baseline so that when we adjust the pixel coordinate of bars we don't remove one that was previously touching baseline. We "anchor" the bars on this baseline
| * | ||
| * @default 0 for value axes, undefined for category axes | ||
| */ | ||
| baseline?: number; |
There was a problem hiding this comment.
This functionality means that someone can set baseline on yAxis and achieve something like this
| Before | After (with baseline on yAxis) |
|---|---|
 |
 |
 |
 |
| if (baseline < bounds.min) return { ...bounds, min: baseline }; | ||
| if (baseline > bounds.max) return { ...bounds, max: baseline }; | ||
| return bounds; | ||
| }; |
There was a problem hiding this comment.
A bug we had before - our AreaChart and BarChart both are meant to include 0 as their 'baseline. This is intentional and documented, with the purpose being this is how most charts work, especially Bar. Line chart is different, with the default matching how most stocks work.
However, our previous baseline logic including of 0 for Area/Bar only worked when the values were positive - if the bottom of the domain was below 0 it wouldn't work. This is problematic for many cases, such as showing gain/loss on a bar chart.
| Before | After |
|---|---|
 |
 |
 |
 |
This has been fixed + this can now be customized by users by adjusting the 'baseline'! Also, users can override the domain/range of axes whenever desired.
| fillOpacity = 1, | ||
| peakOpacity = 0.3, | ||
| baselineOpacity = 0, | ||
| baseline, |
There was a problem hiding this comment.
Baseline hasn't been working in this manner
I believe it broke as soon as we added support for stacking, since stacking logic relies on the baseline value to be predetermined, so adding it on the fly when rendering is too late in the logic pipeline to work.
However now it works when you set it on the axis
| * Domain limit type for numeric scales | ||
| * From axis props when set. Meaningful when this axis is the chart’s value axis for the current layout. | ||
| */ | ||
| domainLimit?: 'nice' | 'strict'; |
There was a problem hiding this comment.
This was already set on AxisConfig (as required), no need to set it again. It can be required since customers are able to pass in partial.
| if (!series) return new Map<string, Array<[number, number] | null>>(); | ||
| return calculateStackedSeriesData(series); | ||
| }, [series]); | ||
| return calculateStackedSeriesData(series, layout, xAxisConfig, yAxisConfig); |
There was a problem hiding this comment.
It now needs this to get the baseline.
| let stackedMax = 0; | ||
| let stackedMin = 0; | ||
| let stackedMax = -Infinity; | ||
| let stackedMin = Infinity; |
There was a problem hiding this comment.
It doesn't make sense to use '0' now that baseline can be any value.
| * @param id - The axis ID. Defaults to defaultAxisId. | ||
| */ | ||
| getYAxis: (id?: string) => AxisConfig | undefined; | ||
| getYAxis: (id?: string) => CartesianAxisConfig | undefined; |
There was a problem hiding this comment.
Not sure why we weren't already using this config for CartesianChart context
| yAxisId: s.yAxisId, | ||
| stackId: s.stackId, | ||
| gradient: s.gradient, | ||
| legendShape: s.legendShape, |
| yAxis?: | ||
| | Partial<Omit<CartesianAxisConfigProps, 'data'>> | ||
| | Partial<Omit<CartesianAxisConfigProps, 'data'>>[]; | ||
| yAxis?: Partial<CartesianAxisConfigProps> | Partial<CartesianAxisConfigProps>[]; |
There was a problem hiding this comment.
We missed this before, data prop is now supported on yAxis,
| yAxis?: | ||
| | Partial<Omit<CartesianAxisConfigProps, 'data'>> | ||
| | Partial<Omit<CartesianAxisConfigProps, 'data'>>[]; | ||
| yAxis?: Partial<CartesianAxisConfigProps> | Partial<CartesianAxisConfigProps>[]; |
There was a problem hiding this comment.
This should have been changed when we enabled support for 'layout'
|
|
||
| const { xAxes, xScales } = useMemo(() => { | ||
| const axes = new Map<string, AxisConfig>(); | ||
| const axes = new Map<string, CartesianAxisConfig>(); |
There was a problem hiding this comment.
This should have been done before as well
2 participants
What changed? Why?
This PR adds "baseline" to chart axes
The concept of "baseline" is that this is the base value that the chart stems from. By default it is 0, meaning bars rendered on the screen will start at 0 and go up or down towards their actual value:
While this had been working before, setting a custom baseline was not working, now it is possible
This works by fixing all area and bar components towards the baseline, and stacking also is in this same manner.
Stacking logic works as such
This means that if we have a stack such as
series={[{ id: 'a', data: [4, -2, 3] }, { id: 'b', data: [5, 1, 2] }]}With a baseline of 0 we'd get [{ positive: 9, negative: 0 }, { positive: 1, negative: -2}, { positive: 7, negative: 0 }]
With a baseline of 3 we'd get [{ positive: 3, negative: 0 }, { positive: 0, negative: 7 }, { positive: 0, negative: 1 }] - keep in mind these values are from "3", so a negative of "7" really equals -4.
Root cause (required for bugfixes)
When we added stacking to all series we didn't factor in that eliminated the ability to use 'baseline' on Area and Bar. This removes that functionality from affected components and brings in a new centralized baseline property which can be customized on axes (on regular charts this is for the y axis for horizontal layout charts it is the x axis.
UI changes
Testing
How has it been tested?
Testing instructions
Illustrations/Icons Checklist
Required if this PR changes files under
packages/illustrations/**orpackages/icons/**Change management
type=routine
risk=low
impact=sev5
automerge=false