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] Use new text component to avoid tick label overflow on x-axis #10648

Merged
merged 25 commits into from
Oct 19, 2023

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Oct 12, 2023

Part of #10140 and #7801

This PR introduces multiple concepts:

  1. the tick selection: You can now select which tick to display. For that, you can provide a function similar to a filtering method

  2. the tick label selection: You can display labels on a subset of ticks. The same method, you provide a filtering method. But it is applied on the result of the tick filtering. In other words you can not get a label without a tick.

  3. automatic label filtering: By default, we keep all ticks, and we display the first label. Then we display the next label that does not overflow, and so on.

  4. The ability to rotate the text

  5. Pass all the customization in the style object.

  • The advantage is that it feels more natural. You can pass the textBaseline as a standard CSS property.
  • The drawback is it mixes some CSS properties that will be passed to the element, and some custom ones such as textBaseline or rotate that are used for the custom <ChartsText /> computation and then restricted to a subset of options.

Tech aspect:

To know if a label overflows (especially if they are rotated), we compute its bounding box, and we get interested by how much do we need to translate them to avoid the overflow (the red line)

For small angles, I approximate them to 0° or 90°.

Hypothesis are: all text boxes are the same height, and angles are between -90° and 90°. Without those hypotheses, computing this minimal gap would require getting the dimension of the next rectangle.

image

Here is a demo where each rectangle is aligned according to the minimal translation to avoid overflow. You can see that when width are different it fails for angles larger than 90° But not sure it is worth fixing it since I assume nobody displays labels rotated by 180°

rotation.mp4

@mui-bot
Copy link

mui-bot commented Oct 12, 2023

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

Updated pages:

Generated by 🚫 dangerJS against 34f0985

@alexfauquette alexfauquette changed the title [charts] Fix typo between internal/external variable [charts] Use new text component to avoid tick label overflow Oct 12, 2023
@alexfauquette alexfauquette marked this pull request as ready for review October 13, 2023 11:38
@alexfauquette alexfauquette changed the title [charts] Use new text component to avoid tick label overflow [charts] Use new text component to avoid tick label overflow on x-axis Oct 13, 2023
@zannager zannager added the component: charts This is the name of the generic UI component, not the React module! label Oct 13, 2023
[`.${axisClasses.bottom}`]: {
[`.${axisClasses.tickLabel}`]: {
transform: 'rotate(45deg)',
xAxis={[
Copy link
Member Author

Choose a reason for hiding this comment

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

Basically here all the styling is moved in the axis properties

Comment on lines +166 to +167
dominantBaseline: 'central',
textAnchor: 'start',
Copy link
Member Author

Choose a reason for hiding this comment

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

Those properties moved into the style prop

@flaviendelangle
Copy link
Member

🤩 That's some impressive evolution!

But not sure it is worth fixing it since I assume nobody displays labels rotated by 180°

Maybe we can just refuse angles we don't handle correctly (either throwing an error or displaying an console warning).

@alexfauquette
Copy link
Member Author

Just noticed an issue remains: When the axis automatically removes some tick labels, it leads to a hydration error. Not sure about how to deal with it. Should the computation of the tick label to remove be done after the first render 🤔

@flaviendelangle
Copy link
Member

flaviendelangle commented Oct 16, 2023

I guess it's because on SSR you don't have a notion of component width so we can't guess the ticks to render.

For me either you refuse the usage of the automatic mode on app that use SSR
Or you have an imperfect fallback where the tick labels changes after hydration (but it causes a flicker so the automatic mode should probably never be the default behavior, just a fallback)

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.

Great work! 👍

Leaving some nitpick comments, functional change suggestions and documentation improvement proposals.

docs/data/charts/axis/axis.md Outdated Show resolved Hide resolved
docs/data/charts/axis/axis.md Outdated Show resolved Hide resolved
docs/data/charts/axis/axis.md Outdated Show resolved Hide resolved
docs/data/charts/axis/axis.md Outdated Show resolved Hide resolved
docs/data/charts/axis/axis.md Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsXAxis/ChartsXAxis.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsXAxis/ChartsXAxis.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsXAxis/ChartsXAxis.tsx Outdated Show resolved Hide resolved
packages/x-charts/src/ChartsXAxis/ChartsXAxis.tsx Outdated Show resolved Hide resolved
docs/data/charts/axis/axis.md 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.

Superb improvement! 💙 🚀

packages/x-charts/src/models/axis.ts Outdated Show resolved Hide resolved
packages/x-charts/src/models/axis.ts Outdated Show resolved Hide resolved
docs/data/charts/axis/axis.md Show resolved Hide resolved
docs/data/charts/axis/AxisTextCustomizationNoSnap.js Outdated Show resolved Hide resolved
alexfauquette and others added 2 commits October 19, 2023 09:33
Co-authored-by: Lukas <[email protected]>
Co-authored-by: Sam Sycamore <[email protected]>
Signed-off-by: Alexandre Fauquette <[email protected]>
@alexfauquette alexfauquette merged commit 79bbfa6 into mui:master Oct 19, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants