Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[charts] Improve the management of the text (legend only) #10138

Merged
merged 42 commits into from
Oct 11, 2023

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Aug 25, 2023

Fix #9743

The target is to get access to the text size (height and width) to be able to place them nicely.

For now, the new <Text /> component allows to get its size and to support multiline texts.

It's used for

  • axes ticks and labels, but it does not allow customization and does not use the size computation for now
  • legend to allow it to wrap. Here it uses size computation and allows style customization

Discussion

Style

The placement of the legend has changed unintentionally. It now sticks to the border of the SVG instead of the border of the drawing area. Not a big deal, but I don't see one being better than the other

I changed a bit the default styling of legend text. I'm open to suggestions on this topic.

DevExp

For me the legend is much better now, but it opens the question about how to pass style the its labels. I went with labelStyle props. So you can do the following.

slotProps={{
  legend: {
    labelStyle: { fill: 'red',  fontSize: 50 }
  }
}}

image

But it could be done with other options to be able to manage states. But it feels overengineered. I don't know if you have ideas

slotProps={{
  legend: {
    styles: { 
      label: {fill: 'red',  fontSize: 50},
      outlinedLabel: {fill: 'red',  fontSize: 50},
      hiddenLabel: {fill: 'red',  fontSize: 50},
      mark: { ... },
    }
  }
}}

Todo

  • In the legend let texts pick the width they need without having to provide a fixed width
  • In the axis adapt the number of ticks to label size
  • Support characters \n
  • Fix docs spacing
  • Make sure no more CSS is applied to text in the docs

Todo next PRs

  • Allow to override legend marks
  • Allow to customize texts for axes
  • Avoid axes ticks overflow

Trade-offs

There are a lot of styled components in axes, and most of their style will be useless with this <Text/> component. I removed all of them and only kept the root one. The default styling is done by CSS classes

Preview is available in the demo of Legend docs page

Changelog

Breaking changes

The charts have a new text display mechanism.
It adds line break support and avoids overlapping text in the legend.
This comes with some breaking changes.

  • The DOM structure is modified. An intermediary <tspan /> element has been added. This can impact how your style is applied.

    - <text>The label</text>
    + <text><tspan>The label</tspan></text>
  • The top margin has been reduced from 100 to 50 to benefit form the denser legend.

  • To accurately compute the text size and then place it, styling should be provided as a JS object. For example, to set the legend font size, you should do:

    <PieChart
      {/** ... */}
      slotProps={{
        legend: {
          labelStyle: {
            fontSize: 16,
          },
        },
      }}
    />

    Support for other text elements (axis labels and tick labels) will be implemented in follow-up PR.

@alexfauquette alexfauquette added the component: charts This is the name of the generic UI component, not the React module! label Aug 25, 2023
@mui-bot
Copy link

mui-bot commented Aug 25, 2023

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

Updated pages:

Generated by 🚫 dangerJS against 309945b

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 20, 2023
@github-actions
Copy link

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

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 21, 2023
packages/x-charts/src/ChartsLegend/ChartsLegend.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsLegend/ChartsLegend.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsLegend/ChartsLegend.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsLegend/ChartsLegend.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsLegend/ChartsLegend.tsx Outdated Show resolved Hide resolved
@@ -5,64 +5,23 @@ export const AxisRoot = styled('g', {
name: 'MuiChartsAxis',
slot: 'Root',
overridesResolver: (props, styles) => styles.root,
})({
Copy link
Member Author

Choose a reason for hiding this comment

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

All the text alignments are moved from this CSS to the <Text/> props

break;
}

// const transforms = [];
Copy link
Member Author

@alexfauquette alexfauquette Sep 25, 2023

Choose a reason for hiding this comment

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

It is not implemented yet. It will mostly be helpful for axes ticks

@alexfauquette alexfauquette marked this pull request as ready for review September 25, 2023 14:20
Copy link
Contributor

@noraleonte noraleonte left a comment

Choose a reason for hiding this comment

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

Great improvement! 🎉 Much better DX, 💙 the docs improvements
I left a few suggestions for the phrasing in the docs 🙈
About the reduced label font size, I agree that they are a bit too small. I would personally go with a middle ground of 14px. WDYT?

docs/data/charts/legend/legend.md Outdated Show resolved Hide resolved
docs/data/charts/legend/legend.md Outdated Show resolved Hide resolved
docs/data/charts/legend/legend.md Outdated Show resolved Hide resolved
docs/data/charts/legend/legend.md Outdated Show resolved Hide resolved
@LukasTy LukasTy added breaking change enhancement This is not a bug, nor a new feature labels Oct 10, 2023
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Looks great! 💯
Leaving some comments. 😉
The label vertical alignment is the culprit for not approving the PR yet as it seems like it would be a regression as compared to the current version. 🙈

packages/x-charts/src/ChartsLegend/ChartsLegend.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsLegend/ChartsLegend.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsLegend/utils.ts Outdated Show resolved Hide resolved
docs/data/charts/legend/LegendTextStylingNoSnap.js Outdated Show resolved Hide resolved
Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Looks great! 💯
Approving based on: #10138 (comment)
The few left nitpicks are just that - minor improvements.

@alexfauquette alexfauquette merged commit 5cd7872 into mui:master Oct 11, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: charts This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[charts] Wrap Legend labels to avoid overlapping when displaying multiple categories
5 participants