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

[Select][material-ui] Add root class to SelectClasses #38424

Merged
merged 14 commits into from
Aug 21, 2023
1 change: 1 addition & 0 deletions docs/pages/material-ui/api/select.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
],
"styles": {
"classes": [
"root",
"select",
"multiple",
"filled",
Expand Down
1 change: 1 addition & 0 deletions docs/translations/api-docs/select/select.json
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
"variant": { "description": "The variant to use." }
},
"classDescriptions": {
"root": { "description": "Styles applied to the root element." },
"select": {
"description": "Styles applied to {{nodeName}}.",
"nodeName": "the select component <code>select</code> class"
Expand Down
12 changes: 9 additions & 3 deletions packages/mui-material/src/Select/Select.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import * as React from 'react';
import PropTypes from 'prop-types';
import clsx from 'clsx';
import { deepmerge } from '@mui/utils';
import { deepmerge, unstable_composeClasses as composeClasses } from '@mui/utils';
import SelectInput from './SelectInput';
import formControlState from '../FormControl/formControlState';
import useFormControl from '../FormControl/useFormControl';
Expand All @@ -14,10 +14,16 @@ import OutlinedInput from '../OutlinedInput';
import useThemeProps from '../styles/useThemeProps';
import useForkRef from '../utils/useForkRef';
import styled, { rootShouldForwardProp } from '../styles/styled';
import { getSelectUtilityClasses } from './selectClasses';

const useUtilityClasses = (ownerState) => {
const { classes } = ownerState;
return classes;

const slots = {
root: ['root'],
};

return composeClasses(slots, getSelectUtilityClasses, classes);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When using composeClasses, all the classes will be under the root key. Do we know if this might have unintended consequences? For example, when forwarding classes here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this might be the cause of the argos failure 🤔

Copy link
Contributor Author

@sai6855 sai6855 Aug 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, you are right. argos got fixed after adding missing select and icon here, which are used in TablePagination component here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think we might break missing keys in regards to this line:

classes: inputProps ? deepmerge(classes, inputProps.classes) : classes,

For example, "multiple" classes will no longer be forwarded.

Could we add the missing classes and add a test for this? Checks are passing, but if I'm correct, they shouldn't be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All missing classes are actually applied in SelectInput component including multiple class here . shouldn't these be enough 🤔 ? maybe that's why tests aren't failing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since all classes are in SelectInput, i've moved root class also to there.

};

const styledRootConfig = {
Expand Down Expand Up @@ -117,7 +123,7 @@ const Select = React.forwardRef(function Select(inProps, ref) {
},
...(multiple && native && variant === 'outlined' ? { notched: true } : {}),
ref: inputComponentRef,
className: clsx(InputComponent.props.className, className),
className: clsx(InputComponent.props.className, className, classes.root),
DiegoAndai marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove classes.root here as it will be handled down the line on InputBase

Copy link
Contributor Author

@sai6855 sai6855 Aug 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This particular test is failing if classes.root is not added here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weird 🤔 I thought both classes.root and className should've ended here. Where might we be loosing classes.root so it wouldn't end here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, my bad. classes is passed inside inputProps. So adding classes.root to the className here is correct.

Should we remove the root key when passing classes to inputProps here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think we should remove. found a similar issue related to duplicate root class. #34502.

// If a custom input is provided via 'input' prop, do not allow 'variant' to be propagated to it's root element. See https://github.com/mui/material-ui/issues/33894.
...(!input && { variant }),
...other,
Expand Down
14 changes: 14 additions & 0 deletions packages/mui-material/src/Select/Select.spec.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as React from 'react';
import Select, { SelectChangeEvent } from '@mui/material/Select';
import MenuItem from '@mui/material/MenuItem';
import { createTheme } from '@mui/material/styles';

function genericValueTest() {
function handleChangeWithSameTypeAsSelect(event: SelectChangeEvent<number>) {}
Expand Down Expand Up @@ -44,4 +45,17 @@ function genericValueTest() {

// disabledUnderline prop should be available (inherited from InputProps) and NOT throw typescript error
<Select disableUnderline />;

// Tests presence of `root` class in SelectClasses
const theme = createTheme({
components: {
MuiSelect: {
styleOverrides: {
root: {
borderRadius: '8px',
},
},
},
},
});
}
3 changes: 3 additions & 0 deletions packages/mui-material/src/Select/selectClasses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import { unstable_generateUtilityClasses as generateUtilityClasses } from '@mui/
import generateUtilityClass from '../generateUtilityClass';

export interface SelectClasses {
/** Styles applied to the root element. */
root: string;
/** Styles applied to the select component `select` class. */
select: string;
/** Styles applied to the select component if `multiple={true}`. */
Expand Down Expand Up @@ -37,6 +39,7 @@ export function getSelectUtilityClasses(slot: string): string {
}

const selectClasses: SelectClasses = generateUtilityClasses('MuiSelect', [
'root',
'select',
'multiple',
'filled',
Expand Down