Skip to content
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

[material-ui][Chip] Fix clickable line height #45101

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

MachaVivek
Copy link

Added an explicit clickable class to the root element when the chip is clickable

Before changes
WhatsApp Image 2025-01-24 at 01 15 27_5c27c3c8

After changes
WhatsApp Image 2025-01-24 at 01 15 51_fc1fcc05

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

closes #45097

@mui-bot
Copy link

mui-bot commented Jan 23, 2025

Netlify deploy preview

https://deploy-preview-45101--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 5a6b0f7

@zannager zannager added the component: chip This is the name of the generic UI component, not the React module! label Jan 24, 2025
@zannager zannager requested a review from DiegoAndai January 24, 2025 09:38
Comment on lines 106 to 119

// 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
Copy link
Member

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

@ZeeshanTamboli ZeeshanTamboli added bug 🐛 Something doesn't work package: material-ui Specific to @mui/material labels Jan 24, 2025
@aarongarciah aarongarciah changed the title [material-ui][Chip] Fixed Clickable area such that it do not depend on the inline height [material-ui][Chip] Fix clickable line height Jan 24, 2025
@ZeeshanTamboli
Copy link
Member

@DiegoAndai @aarongarciah Some CSS properties, especially text-related ones, inherit by default, like line-height. If we reset line-height, does that mean we should also handle and reset other inherited styles?

Wouldn't it be better for the DataGrid component to handle this instead? When there's a non-text element inside a cell, the DataGrid could explicitly apply line-height: normal.

@aarongarciah
Copy link
Member

@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.

@DiegoAndai
Copy link
Member

DiegoAndai commented Jan 28, 2025

It looks like line-height: normal doesn't maintain the previous style, for example:

Multiline chip demo before:
Screenshot 2025-01-28 at 16 19 12

Multiline chip demo after:
Screenshot 2025-01-28 at 16 19 15

But on the first one there's no line-height, so it should be defaulting to normal 🤔

Do you know what might be going on @aarongarciah?

@ZeeshanTamboli
Copy link
Member

@DiegoAndai The line-height in the before demo seems to be inherited from the docs theme, and this PR resets it.

It’s the same in StackBlitz, where no docs theme is applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: chip This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Chip] Clickable area expands with line-height
6 participants