Skip to content
New issue

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

#

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

Already on GitHub? # to your account

Jump to bottom

[charts] Fix empty series pie chart v7 #16657

Merged
merged 3 commits into from
Feb 20, 2025
Merged

[charts] Fix empty series pie chart v7 #16657

merged 3 commits into from
Feb 20, 2025

Conversation

bernardobelchior
Copy link
Member

@bernardobelchior bernardobelchior commented Feb 19, 2025 •
edited
Loading

Fixes #16655.

This issue does not happen in v8, so I'm only fixing it for v7.

Before:

Screen.Recording.2025-02-19.at.17.37.01.mov

After:

Screen.Recording.2025-02-19.at.17.36.31.mov

@bernardobelchior bernardobelchior added bug 🐛 Something doesn't work component: charts This is the name of the generic UI component, not the React module! labels Feb 19, 2025
@mui-bot
Copy link

mui-bot commented Feb 19, 2025 •
edited
Loading

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

Generated by 🚫 dangerJS against fcd60d8

@JCQuintas JCQuintas added the v7.x label Feb 19, 2025
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 19, 2025 •
edited
Loading

Is this worst a small test? If our tests run in <5ms, it feels like yes, if it's slower, maybe not worth the CI slowness.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 19, 2025 •
edited
Loading

Yeah ok, I think it used to be faster but it's not too bad:

Screen.Recording.2025-02-19.at.22.52.22.mov

pushed (in v8, the code doesn't have this problem)

@oliviertassinari oliviertassinari mentioned this pull request Feb 19, 2025
Closed
@JCQuintas JCQuintas added needs cherry-pick The PR should be cherry-picked to master after merge v8.x and removed v7.x labels Feb 19, 2025
@JCQuintas
Copy link
Member

Is this worst a small test? If our tests run in <5ms, it feels like yes, if it's slower, maybe not worth the CI slowness.

Could be good, we just need to cherry-pick the test in v8 again

@bernardobelchior
Copy link
Member Author

@oliviertassinari didn't realize that you had added a unit test already. I tested that the "No data" overlay is present, which also implicitly tests that the component doesn't throw.

Ran it against v7 without the fix and the test failed.

@bernardobelchior bernardobelchior merged commit 3f23a9e into mui:v7.x Feb 20, 2025
16 checks passed
@github-actions GitHub Actions
Copy link

Cherry-pick PRs will be created targeting branches: v8.x

@bernardobelchior bernardobelchior mentioned this pull request Feb 20, 2025
Merged
@bernardobelchior bernardobelchior deleted the fix-empty-series-pie-chart-v7 branch February 20, 2025 08:50
@bernardobelchior
Copy link
Member Author

Cherry-pick PRs will be created targeting branches: v8.x

Automatic cherry-picked failed, so made it manually: #16663

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@JCQuintas JCQuintas JCQuintas approved these changes

@alexfauquette alexfauquette Awaiting requested review from alexfauquette

Assignees
No one assigned
Labels
bug 🐛 Something doesn't work component: charts This is the name of the generic UI component, not the React module! needs cherry-pick The PR should be cherry-picked to master after merge v8.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bernardobelchior @mui-bot @oliviertassinari @JCQuintas