-
-
Notifications
You must be signed in to change notification settings - Fork 32.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
[Joy] Miscellaneous fixes #31873
[Joy] Miscellaneous fixes #31873
Conversation
@mui/joy: parsed: +2.17% , gzip: +1.17% |
...theme.typography.body1, | ||
fontFamily: theme.vars.fontFamily.body, | ||
fontSize: theme.vars.fontSize.md, | ||
lineHeight: 1, |
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.
Just double checking if this is intended. Shouldn't it be 1rem
?
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.
line-height: 1, means that it is equal to the font-size. I think it is better in this case because the button font-size can vary based on the size
prop. The reason I use line-height 1 is that I can't find a better number otherwise you will see that the text is not aligned center inside the button. It is quite the same issue as in #30240
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.
I see. Thanks for explaining!
...(ownerState.nested && { | ||
display: 'inline-flex', | ||
...(ownerState.startIcon && { | ||
verticalAlign: 'bottom', // to make the text align with the parent's content |
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.
Just double checking. Is it intended that we have this property when ownerState.nested && ownerState.startIcon
? Also, do we not need this when there is endIcon
?
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.
Yes, this is intended. There is no alignment issue if the icon is at the end. You can try to remove the verticalAlign
to see the alignment issue. https://deploy-preview-31873--material-ui.netlify.app/experiments/joy/typography/
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.
Looks great! 😁
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.
Excellent work!
Found several issues from building Strapi design system in #30686.
Improvements
Render texts & links should be easy as possible (less
sx
as much as possible). See the use case in this SandboxI decided to add
startDecorator
&endDecorator
prop to the Typography and Link which is very common use case. Also, create a TypographyContext to make the nested text alignment works well together.Ex 1: to achieve this result, you can nest the Typography together
Ex 2: the start & end decorator
Fixes
TextField
: pass type & name props to InputCheckbox
: pass id & name props to input, flex-shrink set to 0 in case it is used in flexboxListItemDecorator
: able to set the color from the root ListListItemButton
: able to pass customrole
Button
: remove unnecessary--Button-minHeight
variableList
: remove the default background and integrated with Sheet, set nested inset to 0px (it is more common)Avatar
: add font-family body and adjust the fallback sizeSvgIcon
: add--Icon-color
for customization.Input, TextField
: renamestart/endAdornment
tostart/endDecorator
.