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][Select] Apply missing root class #42975

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

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Jul 17, 2024

Docs says .MuiSelect-root is applied to root component but it's not applied to any element (check useEffect code in before sandbox to verify this) . This PR adds root class to root component

Before: https://stackblitz.com/edit/react-mxoqrt?file=Demo.tsx

After: https://codesandbox.io/s/quirky-wind-zpyc6c?file=/src/Demo.tsx

@sai6855 sai6855 marked this pull request as draft July 17, 2024 05:08
@sai6855 sai6855 added bug 🐛 Something doesn't work component: select This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Jul 17, 2024
@mui-bot
Copy link

mui-bot commented Jul 17, 2024

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 1, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 8, 2024
@sai6855 sai6855 marked this pull request as ready for review August 8, 2024 09:53

const composedClasses = composeClasses(slots, getSelectUtilityClasses, classes);

return { ...classes, ...composedClasses };
Copy link
Contributor Author

@sai6855 sai6855 Aug 8, 2024

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

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

Copy link
Member

@DiegoAndai DiegoAndai left a 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?

@sai6855
Copy link
Contributor Author

sai6855 commented Aug 21, 2024

@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, root was manually added on line 225 and presence of root class was tested on line 234. If line 225 was removed, line 234 test wouldn't have passed

it('applies the root class to the root component if it has this class', async () => {
const { classes, render, skip } = getOptions();
if (classes.root == null) {
return;
}
const className = randomStringValue();
const classesRootClassname = randomStringValue();
const { container } = await render(
React.cloneElement(element, {
className,
classes: { ...classes, root: `${classes.root} ${classesRootClassname}` },
}),
);
// we established that the root component renders the outermost host previously. We immediately
// jump to the host component because some components pass the `root` class
// to the `classes` prop of the root component.
// https://github.com/mui/material-ui/blob/f9896bcd129a1209153106296b3d2487547ba205/packages/material-ui/src/OutlinedInput/OutlinedInput.js#L101
expect(container.firstChild).to.have.class(className);
expect(container.firstChild).to.have.class(classes.root);
expect(document.querySelectorAll(`.${classes.root}`).length).to.equal(1);

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: select 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.

None yet

4 participants