[charts] Add minBarSize to prevent bars from having 0 height
#18798
Conversation
|
Deploy preview: https://deploy-preview-18798--material-ui-x.netlify.app/ Updated pages: Bundle size reportTotal Size Change: 🔺+2.54KB(+0.02%) - Total Gzip Change: 🔺+1.16KB(+0.03%) Show details for 100 more bundles (22 more not shown)@mui/x-charts parsed: 🔺+250B(+0.08%) gzip: 🔺+116B(+0.13%) |
CodSpeed Performance ReportMerging #18798 will not alter performanceComparing Summary
|
|
|
||
| // Size is above the minimum size, so we can return the bar size and the min value coordinate. | ||
| if (!isSizeLessThanMin) { | ||
| return [barSize, minValueCoord]; |
There was a problem hiding this comment.
Why return an array instead of an object? This isn't a hook nor is it clear the order of the elements. It'd make sense if it were [x, y], for example.
| @@ -61,6 +60,8 @@ const seriesProcessor: SeriesProcessor<'bar'> = (params, dataset) => { | |||
| completedSeries[id] = { | |||
| layout: 'vertical', | |||
| labelMarkType: 'square', | |||
| minBarSize: 0, | |||
| valueFormatter: series[id].valueFormatter ?? ((v) => (v == null ? '' : v.toLocaleString())), | |||
There was a problem hiding this comment.
Could we move the default value formatter outside so that it's a stable function? Should help reduce re-renders.
| setMinBarSize(newValue); | ||
| }; | ||
|
|
||
| return ( |
There was a problem hiding this comment.
If the min bar size is too great, the bars start growing from the top:
Screen.Recording.2025-07-14.at.15.12.33.mov
Do we want to handle this case?
Also, should minBarSize impact what's visible? For example, if due to minBarSize a bar isn't totally visible, should we update the y-axis? In other words, should minBarSize somehow affect the range so that it's always visible (unless min/max are specified on the y-axis)?
There was a problem hiding this comment.
I don't understand what you mean by this. The prop is only changing the visual representation of the bar
There was a problem hiding this comment.
Do we want to handle this case?
We could accept px or percentage, otherwise I would leave it as is.
There was a problem hiding this comment.
I don't understand what you mean by this. The prop is only changing the visual representation of the bar
In the video, the bar for "Tue" moves from starting at 0 to starting at around 5 once the minBarSize increases from 117px to 139px. Is that intended?
There was a problem hiding this comment.
Ah, no that's weird, I'm looking into it
There was a problem hiding this comment.
I was using the wrong param, should be fixed now
| @@ -35,6 +35,12 @@ export interface BarSeriesType | |||
| * @default 'diverging' | |||
| */ | |||
| stackOffset?: StackOffsetType; | |||
| /** | |||
| * If provided, this will be used as the minimum size of the bar. | |||
| const isSizeLessThanMin = Math.abs(maxValueCoord - minValueCoord) < minBarSize; | ||
| const barSize = isSizeLessThanMin ? minBarSize : Math.abs(maxValueCoord - minValueCoord); |
There was a problem hiding this comment.
Before we didn't need Math.abs, why do we need it now? Isn't maxValueCoord always greater than minValueCoord, so the result is always non-negative?
There was a problem hiding this comment.
Not needed yeah, we already run math.min/math.max to get the coords
| minBarSize: number, | ||
| ): { barSize: number; startCoordinate: number } { | ||
| const isSizeLessThanMin = maxValueCoord - minValueCoord < minBarSize; | ||
| const barSize = isSizeLessThanMin ? minBarSize : maxValueCoord - minValueCoord; |
There was a problem hiding this comment.
| const barSize = isSizeLessThanMin ? minBarSize : maxValueCoord - minValueCoord; | |
| const barSize = Math.max(minBarSize, maxValueCoord - minValueCoord); |
|
@JCQuintas I see two main edge cases in this PR
Either we accept this limitation but should mark it as a warning. Or we move the the computation at the stacking level - .value((d, key) => d[key] ?? 0) // defaultize null value to 0
+ .value((d, key) => !d[key] ? 0 : Math.max(minBarValue : d[key])) // defaultize null value to 0 |
|
@alexfauquette I don't think it makes much sense to allow this to work on stacked series. I've added a pr here with the 0/null changes: #18816 |
This guy might disagree 🙈 But Recharts does not support it too. It's doing the same stuff as you rimplementation. But I'm good with leaving that for later |
Fixes #18777