-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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] Remove labelFontSize
and tickFontSize
props
#15633
Conversation
Deploy preview: https://deploy-preview-15633--material-ui-x.netlify.app/ Updated pages: |
CodSpeed Performance ReportMerging #15633 will degrade performances by 24.1%Comparing Summary
Benchmarks breakdown
|
@@ -104,6 +100,8 @@ function ChartsYAxis(inProps: ChartsYAxisProps) { | |||
|
|||
const positionSign = position === 'right' ? 1 : -1; | |||
|
|||
const tickFontSize = 12; |
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.
What about keeping the default behavior if user provides a number?
const tickFontSize = 12; | |
const tickFontSize = typeof tickLabelStyle.fontSize ==== 'number' ? tickLabelStyle.fontSize : 12; |
Or make it a constant outside of the component named defaultTickFontSize
because otherwise it looks like it's the actual one whereas the user could modify it
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.
Nice catch, I've added your suggestion, but we could also parse strings if necessary with something like
function parseTickFontSize(fontSize: string | number) {
if (typeof fontSize === 'number') return fontSize
if (fontSize.includes('px') || /^[0-9]*$/.test('fontSize')) return parseInt(fontSize)
return 12
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.
Argos changed because we were never taking tickLabelStyle.fontSize
into account in the calc 😅
Very clean code mode 👍 |
Co-authored-by: Alexandre Fauquette <[email protected]> Signed-off-by: Jose C Quintas Jr <[email protected]>
The PR close the deprecation issue. At le ast I don't see other deprecated elements :) |
Signed-off-by: Jose C Quintas Jr <[email protected]> Co-authored-by: Alexandre Fauquette <[email protected]>
Props were deprecated, removing them.
Part of #13820
Changelog
labelFontSize
andtickFontSize
props. They were previously deprecated and have now been removed — Learn more