[charts] Add `label` to be displayed inside bars in BarChart #12988

JCQuintas · 2024-05-03T16:24:49Z

Add possibility of showing a label inside bars.

https://deploy-preview-12988--material-ui-x.netlify.app/x/react-charts/bars/#labels

<BarChart
  series={[
    { data: [4, 2, 5, 4, 1], stack: 'A', label: 'Series A1' },
    { data: [2, 8, 1, 3, 1], stack: 'A', label: 'Series A2' },
    { data: [14, 6, 5, 8, 9], label: 'Series B1' },
  ]}
  barLabel={(item, context) => {
    if (item.value > 10) {
      return 'High';
    }
    return context.bar.height < 60 ? null : item.value?.toString();
  }}
  width={600}
  height={350}
/>

Resolves #12331

mui-bot · 2024-05-03T16:28:26Z

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

Updated pages:

docs/data/charts/bars/bars.md

Generated by 🚫 dangerJS against c4fdb79

github-actions · 2024-05-07T12:17:43Z

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

JCQuintas · 2024-05-07T17:03:16Z

@LukasTy can you take a look at this draft and let me know if there are any concerns?

I'm also interested on input on the barLabel property. Right now I went with the signature

barLabel?: (item: BarItem, context: BarLabelContext) => string | null | undefined;

export type BarItem = {
  seriesId: SeriesId;
  dataIndex: number;
  value: number | null;
};

export type BarLabelContext = {
  bar: {
    height: number;
    width: number;
  };
};

this is in contrast with arcLabel, which accepts direct "value mappings" like 'value' | 'label', which would be an option here.

In my solution I went for simplicity, a single property where you can define a function to handle both minheight/width and the value formatting, at the cost of maybe readability?

While the alternative, barLabelMinHeight, barLabelMinWidth, barLabel:'value'|'formattedValue'| Function requires more props, but are "straight forward" allowing for the functionality to be setup without functions if wanted.

LukasTy

Superb addition! 💙 💯
Great work.

LukasTy can you take a look at this draft and let me know if there are any concerns?

I'm also interested on input on the barLabel property.

I like the function signature, it seems both simple and powerful enough. 👍

this is in contrast with arcLabel, which accepts direct "value mappings" like 'value' | 'label', which would be an option here.

IMHO, having that option would improve the feature as I imagine that at least 1/3 of users would end up just using value or label here. 🙈 🤷 😆

packages/x-charts/src/BarChart/BarLabel/types.tsx

packages/x-charts/src/BarChart/BarLabel/barLabelClasses.tsx

packages/x-charts/src/BarChart/BarLabel/BarLabelPlot.tsx

packages/x-charts/src/BarChart/BarLabel/BarLabel.tsx

JCQuintas · 2024-05-10T11:31:09Z

Superb addition! 💙 💯 Great work.

LukasTy can you take a look at this draft and let me know if there are any concerns?

I'm also interested on input on the barLabel property.

I like the function signature, it seems both simple and powerful enough. 👍

this is in contrast with arcLabel, which accepts direct "value mappings" like 'value' | 'label', which would be an option here.

IMHO, having that option would improve the feature as I imagine that at least 1/3 of users would end up just using value or label here. 🙈 🤷 😆

@LukasTy btw I noticed I did it probably wrong? I added the barLabel property to the BarChart/Plot but other properties like this are on the series eg: series[].{type:"pie", arcLabel: ...}. Personally I prefer the props being in the component rather than on the series as I believe series should "pure-ish" data only. But it would probably be better to align the current behaviour in a single place and only change if we decide to do anything about it.

What do you think about that?

LukasTy · 2024-05-10T11:39:22Z

What do you think about that?

Great clarification. Indeed, I missed that part. 🙈
I'd vote for consistency on this regard, even though my first instinct would also be to put such a prop on the BarChart props, but maybe it was done like that to avoid differences when using built-in wrappers and composition? In this case, you'd have to remember to put the barLabel prop directly on the BarPlot, instead of BarChart.
While with the "everything in series" approach there is no difference in where you put these data-related props. 🤔

JCQuintas · 2024-05-10T11:55:09Z

What do you think about that?

Great clarification. Indeed, I missed that part. 🙈 I'd vote for consistency on this regard, even though my first instinct would also be to put such a prop on the BarChart props, but maybe it was done like that to avoid differences when using built-in wrappers and composition? In this case, you'd have to remember to put the barLabel prop directly on the BarPlot, instead of BarChart. While with the "everything in series" approach there is no difference in where you put these data-related props. 🤔

It shouldn't prevent any of the current behaviours I believe, but I don't know, hence why I'm probing for information. In theory, if we threat data as pure, it means it can be used anywhere, which would allow us to use the same "series/datasets" between multiple charts or compositions as we wish. Eg: right now series are "locked" for each type of chart, which is odd to me, if I want to have a BarChart AND a PieChart with the exact same data, I have to duplicate the data? Having a single source of truth for data should allow for better composition imo, but I still don't know the tradeoffs. 😅

Will move it to the series for now

LukasTy · 2024-05-10T11:59:50Z

I have to duplicate the data? Having a single source of truth for data should allow for better composition imo, but I still don't know the tradeoffs. 😅

Will move it to the series for now

I think @alexfauquette should pitch into this discussion with an argument for that particular direction/choice. 😉

JCQuintas · 2024-05-10T12:04:44Z

I have to duplicate the data? Having a single source of truth for data should allow for better composition imo, but I still don't know the tradeoffs. 😅
Will move it to the series for now

I think @alexfauquette should pitch into this discussion with an argument for that particular direction/choice. 😉

Yep, waiting for when he comes back 😄

JCQuintas · 2024-05-13T11:52:20Z

@alexfauquette do you agree with the last few comments? That it looks good that the barLabel was added to the BarChart/BarPlot, but it should probably be in the series[type=bar].barLabel instead? 🤔

I'm going through the process of moving it, but the code will be a bit more complex, so wanted to check before I commit to it 😅

alexfauquette

The overall looks good. I will need to read it again for details. 🎉

About where to put the barLabel topic. For pie chart, I went with series because I had the usecase of multiple nested pie series in mind, where it could make sens to have series with different way of plotting the label. For bar chart it make less sense.

It seems feasible to do every thing with your current approach, even if it ends up looking like:

balLabel={(item) => {
  switch(item.seriesId) ....
}}

packages/x-charts/src/BarChart/BarPlot.tsx

JCQuintas · 2024-05-14T12:51:18Z

The overall looks good. I will need to read it again for details. 🎉

About where to put the barLabel topic. For pie chart, I went with series because I had the usecase of multiple nested pie series in mind, where it could make sens to have series with different way of plotting the label. For bar chart it make less sense.

It seems feasible to do every thing with your current approach, even if it ends up looking like:
balLabel={(item) => {
  switch(item.seriesId) ....
}}

Oops, I just pushed the initial changes, will revert it then 😅

packages/x-charts/src/context/ZAxisContextProvider.tsx

packages/x-charts/src/BarChart/BarLabel/BarLabelPlot.tsx

packages/x-charts/src/BarChart/BarLabel/getBarLabel.ts

packages/x-charts/src/BarChart/BarLabel/BarLabelPlot.tsx

alexfauquette · 2024-05-15T09:14:05Z

packages/x-charts/src/BarChart/BarPlot.tsx


-export interface BarPlotProps extends Pick<BarElementProps, 'slots' | 'slotProps'> {
+export interface BarPlotProps {


Why removing this?

We need to also add the new BarLabelSlots. And typescript complains if we do

export interface BarPlotProps extends Pick<BarElementProps, 'slots' | 'slotProps'>, Pick<BarLabelProps, 'slots' | 'slotProps'> {}

Interface 'BarPlotProps' cannot simultaneously extend types 'Pick<BarElementProps, "slots" | "slotProps">' and 'Pick<BarLabelProps, "slots" | "slotProps">'.

Also it seemed simpler to add them directly based on the above extended props.

Make sens

On this same part of the code, you could use Pick<BarLabelProps, 'barLabel'> to avoididuplicating the type of the barLabel which is just passed From BarPlot to BarLabelPlot

🤔 extending BarLabelProps actually fixes the weird ordering issue #12988 (comment)

So I suppose the import order matters for the script

packages/x-charts/src/BarChart/BarPlot.tsx

packages/x-charts/src/context/ZAxisContextProvider.tsx

alexfauquette

I've a last concern about the exported elements

The <BarLabelPlot /> requires bars which is an array wit all the processing already done. Except if the user is creating a custom BarPlots component, they will never need of this component. And so never need the BarLabel component.

The only component they can reuse for customization purpose is the BarLabelComponent

I would be in favor of not exporting the BarLabelPlot and BarLabel and their props typing. Such that user trying to customize it only have access to the slots which I assume is the good way to do it.

This comment come from reading the BarLabel API page which felt weird because I had no idea about how to use this component, and where do I get the width/height

To prevent exporting internals, you can rely on the diff of scripts/x-charts.exports.json to know what is exported. A common practice could be to export every thing in your files, but only cherry pick what you want to export in src/BarChart/BarLabel/index.ts Normally the export should only take care of depth 1 (src/BarChart/index.ts)

alexfauquette · 2024-05-15T15:04:14Z

scripts/x-charts.exports.json

@@ -266,6 +281,7 @@
  { "name": "useDrawingArea", "kind": "Function" },
  { "name": "useGaugeState", "kind": "Function" },
  { "name": "useSvgRef", "kind": "Function" },
+  { "name": "useUtilityClasses", "kind": "Variable" },


This should not be exported

Did some cleanup, let me know what you think. Also, open to suggestions on BarLabelComponent. We could make it BarLabel only, and rename the current one to something else, open to ideas. 😅

Naming is always an issue with those nested level of components 😅
BarLabel could be renamed

BarLabelComponent (swiching)

BarLabelItem

BarLabelWithContent (we don't care it's internal)

🙃 Types get all funky when changing names.

Also, should owner state be accessible in slotProps? eg:

<BarChart slotProps={{ barLabel: { ownerState: { seriesId: '1', dataIndex: 0, color: 'red', isFaded: false, isHighlighted: false, }, }, }} />

should owner state be accessible in slotProps

I assume yes, user can pass whatever they want, but at their own risk. Why this question?

I was checking which props they could currently pass. It feels odd that they can pass ownerProps.

Also, these "overridable" components from slots shouldn't receive it. 🤔

Like, if we get slots:{bar: (props) => console.log(props) }, we get this object:

{ "style": { "y": 200, "x": 427.1162790697674, "height": 50, "width": 14.883720930232558 }, "cursor": "unset", "ownerState": { "id": "auto-generated-id-3", "dataIndex": 4, "color": "#60009B", "isFaded": false, "isHighlighted": false }, "className": "MuiBarElement-root MuiBarElement-series-auto-generated-id-3" }

For style it probably makes sense, since they are computed. Now, I would argue the ownerState properties should be spread in the props instead of inside ownerState, at least for overridden components, but also slotProps.

The user shouldn't need to know about ownerState

I suppose if we migrate to pigment css it would solve this issue though? 🤔

I re-wrote the files and types, moved BarLabel to BarLabelItem and created a specific file for BarLabel, which applies the theme to the component. I've moved the theme there because eslint was complaining that the component name didn't match (if i left it on BarLabel), but I don't know if the order is correct, like. Which of themeAugmentation vs slotProps should have precedence? 😅

Since you've introduced a component between the styled component and the slots you could pass an empty ownerState to the useSlotProps and pass every info as props. Then create the owner state in the BarLabel component

JCQuintas · 2024-05-15T15:45:35Z

I've a last concern about the exported elements

Thanks for the reply. Yeah I knew something was wrong, but forgot to flag it. One of my intentions with the Code Guidelines docs were to make these boundaries clearer. https://www.notion.so/mui-org/engineering-Code-Guidelines-X-39105ea968754d989af47346fc3bbc3f?pvs=4#2f5425c8d2064cfa8b986ae7b16a28b3

alexfauquette · 2024-05-16T08:00:30Z

I you wonder why the netlify build fail, it's because the API JSON has been removed but you still have the docs/pages/x/api/charts/bar-label.js file looking for it. You should delete it manually

LukasTy

Very nice result. 👏

packages/x-charts/src/BarChart/BarLabel/index.tsx

packages/x-charts/src/BarChart/BarLabel/types.ts

packages/x-charts/src/themeAugmentation/props.d.ts

scripts/x-charts.exports.json

LukasTy

but I don't know if the order is correct, like. Which of themeAugmentation vs slotProps should have precedence? 😅

Anything provided on a specific component usage (i.e. via slotProps) should take precedence over theme augmentation (defaultProps and styleOverrides). 😉

LukasTy · 2024-05-17T09:01:54Z

docs/pages/x/api/charts/spark-line-chart.json

@@ -74,6 +74,12 @@
      "default": "BarElementPath",
      "class": null
    },
+    {
+      "name": "barLabel",


It looks like we are missing the change in SparklineChart to make the barLabel prop work.
Or should it be removed from that component props if we don't want it there? 🤔

I'm in favor of removing it from the props typing. Not very usefull for scatter bar plot, and not sure we want to keep this feature when reworking the sparkline to make it more performant.

NB: José, has you can see having the scripts result in the PR help for the review process 😉

alexfauquette · 2024-05-17T09:01:04Z

packages/x-charts/src/BarChart/BarLabel/types.ts

+  classes?: Partial<BarLabelClasses>;
+}
+
+export type BarItem = {


Just a comment: We could start having those XxxItem types which will be a good complement with the XxxItemIdentifier in the models/seriesType/

I don't get what you mean 🤔

I didn't see that type as well, but I'm not sure the type:'bar' is necessary here

I didn't see that type as well

That's the point I wanted to point this type such that you know they exist for all charts.

but I'm not sure the type:'bar' is necessary here

I agree it was just to share information

alexfauquette · 2024-05-17T09:26:35Z

docs/pages/x/api/charts/spark-line-chart.json

@@ -74,6 +74,12 @@
      "default": "BarElementPath",
      "class": null
    },
+    {
+      "name": "barLabel",


I'm in favor of removing it from the props typing. Not very usefull for scatter bar plot, and not sure we want to keep this feature when reworking the sparkline to make it more performant.

NB: José, has you can see having the scripts result in the PR help for the review process 😉

alexfauquette · 2024-05-17T09:31:12Z

scripts/x-charts.exports.json

@@ -266,6 +281,7 @@
  { "name": "useDrawingArea", "kind": "Function" },
  { "name": "useGaugeState", "kind": "Function" },
  { "name": "useSvgRef", "kind": "Function" },
+  { "name": "useUtilityClasses", "kind": "Variable" },


Since you've introduced a component between the styled component and the slots you could pass an empty ownerState to the useSlotProps and pass every info as props. Then create the owner state in the BarLabel component

JCQuintas · 2024-05-21T09:25:51Z

@alexfauquette regarding ownerProps, do you think simply destructuring ownerState into the BarLabel would suffice? d1da5f2

Everything seems to work fine, and end users should be able to use the props they need.

On another note, BarLabelProps contains all HTML props. Should we start thinking of using a BarLabelOwnProps or something similar to allow users to quickly create an easily usable component? Not for this MR, but for the future probably 🤔

const MyComp = (props: BarLabelOwnProps) => {}

type BarLabelOwnProps = {
  children: ...
  color: ...
  isFaded: ...
  isHighlighted: ...
}

type BarLabelProps = React.SVGProps<SVGTextElement> & BarLabelOwnProps

alexfauquette · 2024-05-21T14:12:48Z

On another note, BarLabelProps contains all HTML props. Should we start thinking of using a BarLabelOwnProps or something similar to allow users to quickly create an easily usable component? Not for this MR, but for the future probably 🤔

What do you mean by "start thinking"? From what I see you're already doing it

JCQuintas · 2024-05-21T14:50:27Z

What do you mean by "start thinking"? From what I see you're already doing it

Sorry, I meant we should think of making this an official pattern with expected naming.

Signed-off-by: Jose C Quintas Jr <[email protected]> Co-authored-by: Lukas <[email protected]> Co-authored-by: Alexandre Fauquette <[email protected]>

jvskriubakken · 2024-09-10T12:20:41Z

When stacking series/bars on top of each other, it becomes difficult to see the total sum of the stacked values. Is it already possible or would you guys consider displaying a total on top of the stacked bars?

Otherwise thanks a lot for mui charts! It's great! Have been wanting charts from MUI for years - and now finally it's here ❤️

alexfauquette · 2024-09-10T14:24:10Z

@jvskriubakken It's not yet feasible. You can follow this issue to be aware about progress on the topic #12975

If the issue does not completely describe your need, feel free to add comments, or open a new one to highlight missing part

JCQuintas added new feature New feature or request component: charts This is the name of the generic UI component, not the React module! labels May 3, 2024

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 7, 2024

JCQuintas force-pushed the 12331-charts-add-label-for-bar-chart branch from eb70e5a to e07c630 Compare May 7, 2024 12:39

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 7, 2024

LukasTy reviewed May 9, 2024

View reviewed changes

alexfauquette reviewed May 14, 2024

View reviewed changes

packages/x-charts/src/BarChart/BarPlot.tsx Outdated Show resolved Hide resolved

JCQuintas force-pushed the 12331-charts-add-label-for-bar-chart branch from 9c64bc1 to 5faedc8 Compare May 14, 2024 12:46

JCQuintas marked this pull request as ready for review May 15, 2024 08:47

JCQuintas requested review from LukasTy and alexfauquette May 15, 2024 08:47

JCQuintas force-pushed the 12331-charts-add-label-for-bar-chart branch from 4845f3b to 3947356 Compare May 15, 2024 08:57

JCQuintas commented May 15, 2024

View reviewed changes

packages/x-charts/src/context/ZAxisContextProvider.tsx Outdated Show resolved Hide resolved

alexfauquette reviewed May 15, 2024

View reviewed changes

JCQuintas added 2 commits May 16, 2024 11:52

Add initial BarElementLabel

84afe67

Ignore types

2ccbfba

JCQuintas added 4 commits May 16, 2024 11:52

Fix typescript error

36efc62

Cleanup exported types

5cd058f

Remove bar-label.js

9efec8d

Add proper svg text element

32ce464

LukasTy reviewed May 16, 2024

View reviewed changes

JCQuintas added 4 commits May 16, 2024 17:26

index.tsx to index.ts

b25d8ac

Use immediate exports

6a70447

Export BarLabel for users

2488988

Improve user api

f89cd77

JCQuintas force-pushed the 12331-charts-add-label-for-bar-chart branch from 139f471 to f89cd77 Compare May 16, 2024 16:46

Fix proptypes

e59975c

LukasTy reviewed May 17, 2024

View reviewed changes

alexfauquette reviewed May 17, 2024

View reviewed changes

JCQuintas added 3 commits May 21, 2024 10:22

rename types file

4d4a0f1

Remove barLabel from sparkline

205a707

Pass ownerstate props as direct props to final component

d1da5f2

alexfauquette mentioned this pull request May 21, 2024

[charts] Add an option to put bar label on top of the bar #12975

Open

Refactor BarLabel component PropTypes

c4fdb79

alexfauquette approved these changes May 21, 2024

View reviewed changes

JCQuintas merged commit 5585ea1 into mui:master May 21, 2024
17 checks passed

JCQuintas deleted the 12331-charts-add-label-for-bar-chart branch May 21, 2024 14:50

Rossen27 mentioned this pull request Jun 22, 2024

[Snyk] Upgrade @mui/x-tree-view from 7.3.1 to 7.6.1 Rossen27/Shopit-V1#81

Open


		export interface BarPlotProps extends Pick<BarElementProps, 'slots' \| 'slotProps'> {
		export interface BarPlotProps {

[charts] Add label to be displayed inside bars in BarChart #12988

[charts] Add label to be displayed inside bars in BarChart #12988

Conversation

JCQuintas commented May 3, 2024 • edited Loading

mui-bot commented May 3, 2024 • edited Loading

github-actions bot commented May 7, 2024

JCQuintas commented May 7, 2024

LukasTy left a comment • edited Loading

Choose a reason for hiding this comment

JCQuintas commented May 10, 2024

LukasTy commented May 10, 2024

JCQuintas commented May 10, 2024 • edited Loading

LukasTy commented May 10, 2024

JCQuintas commented May 10, 2024

JCQuintas commented May 13, 2024

alexfauquette left a comment

Choose a reason for hiding this comment

JCQuintas commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexfauquette left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JCQuintas commented May 15, 2024

alexfauquette commented May 16, 2024

LukasTy left a comment

Choose a reason for hiding this comment

LukasTy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JCQuintas commented May 21, 2024

alexfauquette commented May 21, 2024

JCQuintas commented May 21, 2024

jvskriubakken commented Sep 10, 2024 • edited Loading

alexfauquette commented Sep 10, 2024

[charts] Add `label` to be displayed inside bars in BarChart #12988

[charts] Add `label` to be displayed inside bars in BarChart #12988

JCQuintas commented May 3, 2024 •

edited

Loading

mui-bot commented May 3, 2024 •

edited

Loading

LukasTy left a comment •

edited

Loading

JCQuintas commented May 10, 2024 •

edited

Loading

jvskriubakken commented Sep 10, 2024 •

edited

Loading