feat: allow customization of axis tick mark and grid line alignment in band scale #291
Conversation
✅ Heimdall Review Status
✅
|
| Code Owner | Status | Calculation | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| ui-systems-eng-team |
✅
1/1
|
Denominator calculation
|
| transition={animate ? { duration: 0.2, ease: 'easeOut' } : undefined} | ||
| transition={ | ||
| animate ? { duration: accessoryFadeTransitionDuration, ease: 'easeOut' } : undefined | ||
| } |
There was a problem hiding this comment.
Want to match the animation of grid lines
| strokeCap="square" | ||
| strokeWidth={1} | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
Even though this functionality is being added to YAxis, there is no feasible way for customers to use a bandscale on y axis at this point in time.
However, this PR is laying the groundwork so we can support horizontal layouts (horizontal bar charts that go from left to right instead of bottom to top).
| } | ||
| ``` | ||
|
|
||
| On band scales, you can also use `bandGridPlacement` to control where grid lines appear relative to each band. |
There was a problem hiding this comment.
I didn't update BarChart's documentation since I plan on revamping the whole component doc in my next PR involving this work, where I hope to add support for layout="horizontal" to bar charts.
| }, | ||
| export const axisUpdateAnimationTransition = { | ||
| duration: accessoryFadeTransitionDuration, | ||
| }; |
There was a problem hiding this comment.
We weren't correctly doing this animation on mobile so I removed it. The initial goal for this functionality was to nicely fade in and out grid lines coinciding with text as that previously wouldn't show immediately.
This is still the case on web but with our new mobile library we can immediately show text, since we are able to synchronously measure the bounding box of it with Skia.
|
|
||
| import { useCartesianChartContext } from '../ChartProvider'; | ||
| import { DottedLine } from '../line/DottedLine'; | ||
| import { ReferenceLine } from '../line/ReferenceLine'; |
There was a problem hiding this comment.
We previously used ReferenceLine to render the grid lines but this is no longer possible as we need to target a specific anchor within a band to render the lines at. ReferenceLine will always render in the middle of the band.
| default: | ||
| return 'middle'; | ||
| } | ||
| }; |
There was a problem hiding this comment.
I believe having these two decoupled would be best, AxisBandPlacement and PointAnchor. PointAnchor could allow for more offsets in the future if we decide on supporting more scale types but these placements within the axis would likely not support those other positions.
| position: position + ((scaleFunction as any).bandwidth?.() ?? 0) / 2, | ||
| }; | ||
| }) | ||
| .filter(Boolean) as Array<{ tick: number; position: number }>; |
There was a problem hiding this comment.
We are able to simplify several spots where we needed to manually find the middle of the band before.
| .range([range.min, range.max]) | ||
| .padding(padding); | ||
| .paddingInner(padding) | ||
| .paddingOuter(padding / 2); |
There was a problem hiding this comment.
This change makes us have even spacing between each edge of the step and the band, meaning two bars next to each other now have twice as much space as the last bar has to the edge of the drawing area.
Before
After
This matches other libraries, such as MUI
There was a problem hiding this comment.
I wonder if this can be customized (or is it a strong enough use case for us to enable this customization?). If customer prefer the old outer padding can they switch it back?
There was a problem hiding this comment.
We could potentially open this up, but other libraries hardcode this ratio as well - https://github.com/mui/mui-x/blob/master/packages/x-charts/src/internals/plugins/featurePlugins/useChartCartesianAxis/getAxisScale.ts#L41.
There could be a benefit to allowing custom scales in general in the future, could be a clean way to open this up https://d3js.org/d3-scale/band since they'd get more than just being able to customize padding.
haoruikun-cb
left a comment
haoruikun-cb
left a comment
There was a problem hiding this comment.
Great job! Left some suggestions & question
| .range([range.min, range.max]) | ||
| .padding(padding); | ||
| .paddingInner(padding) | ||
| .paddingOuter(padding / 2); |
There was a problem hiding this comment.
I wonder if this can be customized (or is it a strong enough use case for us to enable this customization?). If customer prefer the old outer padding can they switch it back?
3 participants
What changed? Why?
This PR enables customization of axis grid lines and tick marks when in band scale. Band scale is for discrete data, not continuous. Our bar charts use band scale for the x axis, and can use a continuous scale for y axis (linear or log).
Band scale has bands and steps, where the step portion contains padding. We allow placement in code when using getPointOnScale to be any of these anchor positions (such as stepStart or bandEnd) but only use the step start/end for the grid lines.
Previously our axis grid lines would be placed in the middle of the band:
Now, the default will be to have the grid lines be at the 'edges', which places them at the start and adds one more at the end for the last tick:
This can also be customized by teams.
This feature is found in some other libraries and it is 50/50 on if their default is middle or edges/extremities for the grid lines.
We also support this for tick marks, but I did not add support for tick labels - something we could do in the future if a customer had a use case for it.
UI changes
iOS
Web
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