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] fix y-axis tick label overflow #16846

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

bernardobelchior
Copy link
Member

@bernardobelchior bernardobelchior commented Mar 6, 2025

Part of #10928.

Depends on:

Fixes tick label overflow for the y-axis. It follows the same approach as #16709, but the y-axis doesn't hide overlapping labels at the moment, which is why in the "after" image below you'll see that there's overlapping labels, namely when the labels are rotated -90 and 90 degrees.

The algorithm for hiding overlapping labels in the y-axis is out of scope for this PR.

Before:

localhost_3001_playground_long-y-axis-ticks_ (4)

After:
localhost_3001_playground_long-y-axis-ticks_ (3)

@bernardobelchior bernardobelchior added enhancement This is not a bug, nor a new feature component: charts This is the name of the generic UI component, not the React module! labels Mar 6, 2025
@mui-bot
Copy link

mui-bot commented Mar 6, 2025

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

Generated by 🚫 dangerJS against e14bb2d

Copy link

codspeed-hq bot commented Mar 6, 2025

CodSpeed Performance Report

Merging #16846 will not alter performance

Comparing bernardobelchior:fix-y-axis-tick-overflow (e14bb2d) with master (5fd9c1e)

Summary

✅ 7 untouched benchmarks

@bernardobelchior
Copy link
Member Author

@alexfauquette @JCQuintas should we consider once again updating the default axis size? Now for the y-axis 😛

The default axis size of 30px isn't enough to render "1,000" without ellipsis:

image

To do so, we'd need a width of 42px:

image

Should we increase the axis size? Would it make sense to reduce the margin or should we keep it as is? 🤔

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

github-actions bot commented Mar 7, 2025

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

@bernardobelchior bernardobelchior force-pushed the fix-y-axis-tick-overflow branch from e7fa935 to 1819759 Compare March 12, 2025 12:38
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 12, 2025
@bernardobelchior bernardobelchior force-pushed the fix-y-axis-tick-overflow branch from ce38fac to 5ef090b Compare March 12, 2025 12:50
@bernardobelchior bernardobelchior force-pushed the fix-y-axis-tick-overflow branch from 5ef090b to 9898b1c Compare March 18, 2025 12:06
@bernardobelchior bernardobelchior force-pushed the fix-y-axis-tick-overflow branch from 68a7125 to 0747a89 Compare March 18, 2025 13:27
@bernardobelchior
Copy link
Member Author

bernardobelchior commented Mar 18, 2025

@alexfauquette @JCQuintas should we consider once again updating the default axis size? Now for the y-axis 😛

The default axis size of 30px isn't enough to render "1,000" without ellipsis:

image

To do so, we'd need a width of 42px:

image

Should we increase the axis size? Would it make sense to reduce the margin or should we keep it as is? 🤔

Increased it to 50px and solved the overflows of most demos. Let me know if you think that's acceptable

@JCQuintas
Copy link
Member

Increased it to 50px and solved the overflows of most demos. Let me know if you think that's acceptable

That's probably for the better if it makes it so the defaults are better for everyone, I'll take a look at the screenshots

@JCQuintas
Copy link
Member

Increased it to 50px and solved the overflows of most demos. Let me know if you think that's acceptable

It seems 40 would fit an entire 4 character number, though I suppose it doesn't because there are some margins expected to exist?

@bernardobelchior
Copy link
Member Author

Increased it to 50px and solved the overflows of most demos. Let me know if you think that's acceptable

It seems 40 would fit an entire 4 character number, though I suppose it doesn't because there are some margins expected to exist?

To fit "1,000" we would need 42px as indicated in this comment. Without the comma, 40px might fit, yeah. Do you prefer if we chose 40px? It seems adding a comma is the default format.

@JCQuintas
Copy link
Member

My brain didn't compute the comma 😅

I believe the best approach would be the lowest size we can have that solves most of the cases we have. Ofc my brain would prefer a round number, but If 42 is the lowest, then we might want to use that instead.

@bernardobelchior
Copy link
Member Author

My brain didn't compute the comma 😅

I believe the best approach would be the lowest size we can have that solves most of the cases we have. Ofc my brain would prefer a round number, but If 42 is the lowest, then we might want to use that instead.

Ok, but I'd increase a couple pixels to maybe 45px. Text rendering can be different across different OSes, so it's possible that the same font and same text have different sizes.

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! enhancement This is not a bug, nor a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants