Skip to content

Conversation

@Sigdriv
Copy link
Contributor

@Sigdriv Sigdriv commented Nov 4, 2025

@mui-bot
Copy link

mui-bot commented Nov 4, 2025

Deploy preview: https://deploy-preview-20194--material-ui-x.netlify.app/

Updated pages:

Bundle size report

Bundle Parsed size Gzip size
@mui/x-data-grid 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-pro 0B(0.00%) 0B(0.00%)
@mui/x-data-grid-premium 0B(0.00%) 0B(0.00%)
@mui/x-charts 🔺+934B(+0.28%) 🔺+332B(+0.33%)
@mui/x-charts-pro 🔺+934B(+0.21%) 🔺+320B(+0.24%)
@mui/x-charts-premium 🔺+934B(+0.21%) 🔺+315B(+0.24%)
@mui/x-date-pickers 0B(0.00%) 0B(0.00%)
@mui/x-date-pickers-pro 0B(0.00%) 0B(0.00%)
@mui/x-tree-view 0B(0.00%) 0B(0.00%)
@mui/x-tree-view-pro 0B(0.00%) 0B(0.00%)

Details of bundle changes

Generated by 🚫 dangerJS against d4e7419

@Sigdriv Sigdriv force-pushed the feat/barLabelPlacement branch from 8fb20bc to 7fda73e Compare November 4, 2025 15:08
@bernardobelchior
Copy link
Member

Thanks for your contribution, @Sigdriv! We're currently evaluating whether it makes sense for barLabel to be defined at the chart or at the series level. That decision will probably affect this PR as well, so we might be slower to approve to avoid adding a prop that will be immediately deprecated.

Nevertheless, I think we can already review this PR and deal with that change if needed.

@bernardobelchior bernardobelchior changed the title [charts] Add prop for position of barLabel on BarCharts [charts] Add prop for positioning a bar label Nov 4, 2025
@bernardobelchior bernardobelchior added type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature. scope: charts Changes related to the charts. labels Nov 4, 2025
Copy link
Member

@bernardobelchior bernardobelchior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sigdriv what's the intended behavior when rendering negative bars?

Here's how it looks like now:

image

I'm wondering if the label should be shown below the bar when the bar is negative. What do you think?

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 4, 2025

CodSpeed Performance Report

Merging #20194 will not alter performance

Comparing Sigdriv:feat/barLabelPlacement (8c51094) with master (1938a4a)1

Summary

✅ 13 untouched

Footnotes

  1. No successful run was found on master (86e39e5) during the generation of this report, so 1938a4a was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@bernardobelchior
Copy link
Member

It doesn't seem to be working on an horizontal layout:

image

@Sigdriv
Copy link
Contributor Author

Sigdriv commented Nov 4, 2025

@Sigdriv what's the intended behavior when rendering negative bars?

Here's how it looks like now:

image I'm wondering if the label should be shown below the bar when the bar is negative. What do you think?

Hmm, that's a good question. I think it will look great if we put it down below the bar, but I don't know how we should handle it if the bar is on the axis like in the picture below (see the bar with value -4):

image

@Sigdriv
Copy link
Contributor Author

Sigdriv commented Nov 4, 2025

@bernardobelchior One thing that I haven't thought about is how we should handle the "stacking" functionality with this. The way it's now it will be at the top of the bar, which is not looking good.

Do you have any input on how we can solve this?

image

@Sigdriv
Copy link
Contributor Author

Sigdriv commented Nov 4, 2025

It doesn't seem to be working on an horizontal layout:

image

I looked into this but I don't know how we should do it. If we do it like this I'm afraid that the label will be clipped if the value is to high like 1000

image

@Sigdriv Sigdriv force-pushed the feat/barLabelPlacement branch from 7fda73e to 6f189e1 Compare November 4, 2025 19:58
@Sigdriv Sigdriv force-pushed the feat/barLabelPlacement branch from a00a9d5 to 2198677 Compare November 4, 2025 20:21
@prakhargupta1
Copy link
Member

@bernardobelchior One thing that I haven't thought about is how we should handle the "stacking" functionality with this. The way it's now it will be at the top of the bar, which is not looking good.

Do you have any input on how we can solve this?

image

Just to keep in mind that in future we may have to support showing total at the top of the stacked bar as requested in #19458.

@github-actions
Copy link

github-actions bot commented Nov 5, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Nov 5, 2025
@bernardobelchior
Copy link
Member

Hmm, that's a good question. I think it will look great if we put it down below the bar, but I don't know how we should handle it if the bar is on the axis like in the picture below (see the bar with value -4):

That's an edge case that'll likely also happen if we have an axis on top, so it doesn't happen only in negative bars.

I think we can live with that edge case. There are multiple ways to solve it: use the barLabel to hide the label if it's too close to the limit, decrease the chart's min so that the negative isn't touching the x-axis. We can also consider (as a follow-up) making barLabelPlacement a function that receives information such as value and dataIndex so placement can vary depending on these values.

@bernardobelchior One thing that I haven't thought about is how we should handle the "stacking" functionality with this. The way it's now it will be at the top of the bar, which is not looking good.

Do you have any input on how we can solve this?

That's what happens currently when the bar label is centered, so I think this is a bit out of scope for this PR.

One thing I'm thinking of is that we could add stacking information to the barLabel context. That would allow users to customize this behavior as they please.

I looked into this but I don't know how we should do it. If we do it like this I'm afraid that the label will be clipped if the value is to high like 1000

I think this is a valid solution for now. We can always improve on it later if there are many requests.

We can also consider adding a slot for BarLabelItem so that users can customize the behavior as they please. With a slot they would be able to implement custom truncating behavior if that's something useful to them. We can tackle the slot as a follow-up as well.

@Sigdriv Sigdriv force-pushed the feat/barLabelPlacement branch from 2198677 to 2659553 Compare November 5, 2025 19:13
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Nov 5, 2025
@Sigdriv Sigdriv force-pushed the feat/barLabelPlacement branch from e0d3e86 to 4dcdeff Compare November 6, 2025 07:43
Copy link
Member

@bernardobelchior bernardobelchior left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be an issue with this visual regression test.

@Sigdriv Sigdriv force-pushed the feat/barLabelPlacement branch from 4dcdeff to cb54ae7 Compare November 6, 2025 19:40
@Sigdriv Sigdriv force-pushed the feat/barLabelPlacement branch from 1ea9806 to eabad03 Compare November 10, 2025 14:18
@Sigdriv
Copy link
Contributor Author

Sigdriv commented Nov 10, 2025

I suspect it's related to the changes made to ChartsClipPath. If you revert the changes, does the issue still happen?

I have reverted the changes to the ChartsClipPath and it fixed the issue with the other bar. But now we have the clipping again.

Comment on lines 92 to 93
const shouldPlaceBelow = props.y > props.yOrigin;
const shouldPlaceAbove = props.y < props.yOrigin;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sigdriv based the implementation on the x/y coordinates instead of the bar's value.

The benefits of this approach are that it handles axis reversion gracefully and that we don't need useAnimateBarLabel to depend on value, so there's no breaking change.

The prior implementation looked like this:

Image

Notice that in the layout: vertical, reverse: true case, the -1 and -4 values are rendering below the bar when should be rendering above.

The new implementation fixes the issue:

Image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch

@bernardobelchior
Copy link
Member

@Sigdriv I really wanted to thank you for this work 🙏

I've done one change that I explained here. Also added a regression test to ensure we cover all these cases.

We'll just another review and I think it's good to go.

Amazing work 🎉

@bernardobelchior
Copy link
Member

I have reverted the changes to the ChartsClipPath and it fixed the issue with the other bar. But now we have the clipping again.

Yes, that's expected. Does this affect your use case?

I see several ways to fix it, but I think we should do it as a follow-up:

  1. Move BarLabelPlot outside the clip path (we'd need to check if it doesn't cause any breaking changes)
    a. Optionally, we can also add an option to clip the labels or not
  2. Encourage composition and render BarLabelPlot outside the clip path
  3. Implement some logic to avoid the labels being rendered outside the drawing area, which why they're being clipped. This is harder because we'd need to measure the size of the label to know what offset to apply.

Comment on lines 221 to 222
const minValueCoord = Math.min(...valueCoordinates);
const maxValueCoord = Math.max(...valueCoordinates);
Copy link
Member

@bernardobelchior bernardobelchior Nov 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to trace this change back to this PR. Still need to check what implications this might have, but it's creating issues because we aren't rounded the origin coordinates, making it impossible to compare to the x/y coordinates.

If we still need to round these values, we can do so when rendering instead of doing it here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted because it caused gaps between bars: https://app.argos-ci.com/mui/mui-x/builds/43580/218980533

Opted for rounding the origin instead.

@bernardobelchior
Copy link
Member

@Sigdriv created this issue to track the bar label clipping issue

Copy link
Member

@JCQuintas JCQuintas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice works, there is still a small bug, also left some documentation suggestions


:::info
When using `outside` placement, if the label does not fit in the chart area, it will be clipped.
To avoid this, you can decrease/increase the min/max respectively so that there's enough space for the labels.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which min/max is this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if it's clearer now

@bernardobelchior bernardobelchior merged commit 28f7e77 into mui:master Nov 11, 2025
20 of 21 checks passed
@bernardobelchior
Copy link
Member

Once again, thank you @Sigdriv for the initiative 🙏

@Sigdriv Sigdriv deleted the feat/barLabelPlacement branch November 12, 2025 11:11
bernardobelchior added a commit to bernardobelchior/mui-x that referenced this pull request Nov 19, 2025
bernardobelchior added a commit to bernardobelchior/mui-x that referenced this pull request Nov 19, 2025
bernardobelchior added a commit to bernardobelchior/mui-x that referenced this pull request Nov 19, 2025
bernardobelchior added a commit to bernardobelchior/mui-x that referenced this pull request Nov 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: charts Changes related to the charts. type: enhancement It’s an improvement, but we can’t make up our mind whether it's a bug fix or a new feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants