-
-
Notifications
You must be signed in to change notification settings - Fork 7k
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
Fix: Adjust pie chart title to prevent cropping and added example for testing #6242
base: develop
Are you sure you want to change the base?
Conversation
|
✅ Deploy Preview for mermaid-js ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
commit: |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6242 +/- ##
==========================================
- Coverage 4.47% 4.47% -0.01%
==========================================
Files 385 384 -1
Lines 54180 54221 +41
Branches 598 623 +25
==========================================
Hits 2425 2425
- Misses 51755 51796 +41
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @nourhenta, thanks for the PR, love the dynamic sizing!
We need to make a few changes for it to be merged.
- Please revert all the prettier related changes, these can be added in a separate PR
- Remove changes from unrelated files like classRenderer
Although not a hard requirement, It would be good to split the title into lines, if it's overflowing, even after the min font size is reached.
@@ -1,4 +1,4 @@ | |||
<!--Do not edit this file--> | |||
OB<!--Do not edit this file--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's explicitly said to not edit this file, please revert the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this change
// Calculate the percentage and ensure it is returned as a string | ||
return `${((datum.data.value / sum) * 100).toFixed(0)}%`; | ||
}) | ||
.attr('transform', (datum: d3.PieArcDatum<D3Section>): string => { | ||
// Safely deconstruct coordinates and use template literals for the transform attribute |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are these comments removed?
Description
Fixes #6232 - Pie Chart Title Gets Cut Off if Too Long
Solution
Testing
Tested with different title lengths to ensure proper resizing. Attached a screenshot of the result.
Notes
This solution avoids splitting the title into multiple lines and maintains the design aesthetics by scaling the font size.