-
-
Notifications
You must be signed in to change notification settings - Fork 32.6k
[material-ui][Chip] Fix clickable line height #45101
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
base: master
Are you sure you want to change the base?
[material-ui][Chip] Fix clickable line height #45101
Conversation
Signed-off-by: MachaVivek <[email protected]>
Netlify deploy previewhttps://deploy-preview-45101--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
|
||
// gggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg | ||
'& > *': { | ||
cursor: 'inherit', // Inherit cursor from parent | ||
}, | ||
'&.clickable': { | ||
cursor: 'pointer', | ||
userSelect: 'none', | ||
WebkitTapHighlightColor: 'transparent', | ||
}, | ||
'&.clickable > *': { | ||
pointerEvents: 'none', // Prevent nested elements from capturing pointer events | ||
}, | ||
// gggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggggg |
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 don't think all this logic is needed. We can simply reset inherited line-height
style using line-height: normal
:
--- a/packages/mui-material/src/Chip/Chip.js
+++ b/packages/mui-material/src/Chip/Chip.js
@@ -89,6 +89,7 @@ const ChipRoot = styled('div', {
alignItems: 'center',
justifyContent: 'center',
height: 32,
+ lineHeight: 'normal',
color: (theme.vars || theme).palette.text.primary,
backgroundColor: (theme.vars || theme).palette.action.selected,
borderRadius: 32 / 2,
Docs: https://developer.mozilla.org/en-US/docs/Web/CSS/line-height#normal
@DiegoAndai @aarongarciah Some CSS properties, especially text-related ones, inherit by default, like Wouldn't it be better for the |
@ZeeshanTamboli ideally, a Chip component (and most components) should look the same whenever is placed: standalone, inside a Grid cell, etc. So I think the Chip component should be the one dictating its appearance and being defensive in this regard. So yes, the Chip component should probably explicitly set CSS font properties so they're not overridden by parent elements. |
Signed-off-by: MachaVivek <[email protected]>
It looks like But on the first one there's no Do you know what might be going on @aarongarciah? |
@DiegoAndai The It’s the same in StackBlitz, where no docs theme is applied. |
@aarongarciah should we be more specific with Changing to normal causes the text to drop a bit. In the following image, left is before, right is after, and I added red lines on the before's text. Which looks uncentered, here's the after with green lines on the after's text: You can also see this difference between the current ones and the preview deploy. Note: Not sure if it's best to center considering the x-height and baseline, or the cap height and baseline (terminology source) 🤔 |
@DiegoAndai yes, let's go with specific values. Let's also have into account the small size and multiline to see if it looks good in those scenarios. We might need to tweak the line-height depending on the size to make it look good. PS: I wish the Chip component used values coming from a scale. I see it uses a hardcoded font-size ( |
Added an explicit clickable class to the root element when the chip is clickable
Before changes

After changes

so you can clearly see that click can be shown only when you are on the button
closes #45097