-
-
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
[material-ui][Select] Apply missing root class #42975
base: master
Are you sure you want to change the base?
Conversation
Netlify deploy previewhttps://deploy-preview-42975--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
|
||
const composedClasses = composeClasses(slots, getSelectUtilityClasses, classes); | ||
|
||
return { ...classes, ...composedClasses }; |
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.
This implementation is taken from here
material-ui/packages/mui-material/src/Button/Button.js
Lines 39 to 42 in 75c24b0
...classes, // forward the focused, disabled, etc. classes to the ButtonBase | |
...composedClasses, | |
}; | |
}; |
without spreading .MuiTablePagination-select isn't getting applied correctly and lead to argos errors like these https://app.argos-ci.com/mui/material-ui/builds/29939/99301860
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 wonder if we might be missing something here. There is code to handle the root class: https://github.com/mui/material-ui/pull/42975/files#diff-3b8536ef0001d400b722b173fd9a64c9ce4eb8127bba26f7865977c5bb1a254fR87
Are we sure there's no handling of the root class that we're not seeing?
@sai6855, may I ask you to review when that code in line 87 was added and if it ever worked?
Line 87 was added by me 😑 in this PR #38424, and PR didn't have any test that tests presence of root class on root element. If remember correctly, my reasoning for not adding test was, i thought following test would be good enough. Turs out it's not. Reason this test passed is, material-ui/packages-internal/test-utils/src/describeConformance.tsx Lines 214 to 236 in 6bb06b9
|
@sai6855 @DiegoAndai is this PR still relevant? |
This is still relevant, and reviewing again I think this is the correct solution. Sorry for blocking it before. @sai6855 may I ask you to update the PR? |
@sai6855 to fix the argos build, lets close this PR and open a new one with the same changes. |
opened new PR here #44928 |
Docs says
.MuiSelect-root
is applied toroot
component but it's not applied to any element (check useEffect code inbefore
sandbox to verify this) . This PR addsroot
class to root componentBefore: https://stackblitz.com/edit/react-mxoqrt?file=Demo.tsx
After: https://codesandbox.io/s/quirky-wind-zpyc6c?file=/src/Demo.tsx