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.

Sign up for GitHub

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

Jump to bottom

[TablePagination] Fix select variant not working #33974

Merged
merged 10 commits into from Aug 25, 2022
Merged

[TablePagination] Fix select variant not working #33974

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d01c299
add test cases
ZeeshanTamboli Aug 18, 2022
26c0c7c
apply Select variant if provided
ZeeshanTamboli Aug 18, 2022
7fa23f4
do not attach variant as an attribute to select root div
ZeeshanTamboli Aug 18, 2022
9b51983
Merge branch 'master' into issue/33894-TablePagination-select
ZeeshanTamboli Aug 18, 2022
e18a0eb
update test
ZeeshanTamboli Aug 18, 2022
da058e7
test: parentNode -> parentElement
ZeeshanTamboli Aug 18, 2022
475a225
fix visual regressions by allowing variant in inputProps
ZeeshanTamboli Aug 18, 2022
d41ec9d
add back variant standard
ZeeshanTamboli Aug 18, 2022
7bc5548
add comment
ZeeshanTamboli Aug 25, 2022
9ef1aa8
Merge branch 'master' into issue/33894-TablePagination-select
ZeeshanTamboli Aug 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/mui-material/src/Select/Select.js
View file
Open in desktop
Expand Up @@ -114,7 +114,7 @@ const Select = React.forwardRef(function Select(inProps, ref) {
...(multiple && native && variant === 'outlined' ? { notched: true } : {}),
ref: inputComponentRef,
className: clsx(InputComponent.props.className, className),
variant,
...(!input && { variant }),
Copy link
Member Author

Choose a reason for hiding this comment

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

Do not propagate variant to root div in select input if custom input element is provided via Select's input prop. Basically, do not let variant be set like in the image below:

Capture

Copy link
Member

Choose a reason for hiding this comment

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

Nice, it does not introduce a regression if I do this:

<Select input={<OutlinedInput />}>

So, looks good.

Copy link
Member

Choose a reason for hiding this comment

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

@ZeeshanTamboli I think it deserves a comment. Can you add a comment that explain the consition and maybe link to the issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

@siriwatknp I added the comment.

...other,
});
});
Expand Down
2 changes: 1 addition & 1 deletion packages/mui-material/src/TablePagination/TablePagination.js
View file
Open in desktop
Expand Up @@ -199,7 +199,7 @@ const TablePagination = React.forwardRef(function TablePagination(inProps, ref)
{rowsPerPageOptions.length > 1 && (
<TablePaginationSelect
variant="standard"
input={<InputBase />}
{...(!SelectProps.variant && { input: <InputBase /> })}
Copy link
Member Author

Choose a reason for hiding this comment

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

InputBase should not always be rendered in TablePaginationSelect. If variant is passed through SelectProps prop, that particular variant input should be rendered instead.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder why the <InputBase /> is needed in the first place 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

It's needed. Otherwise, <Input /> is rendered which is different visually. So InputBase should be rendered when no Select variant is provided.

And if variant standard is removed and input={<InputBase />} is kept, there is a slight difference in UI and argos detects changes.

Copy link
Member

Choose a reason for hiding this comment

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

Confirmed, the InputBase is needed.

value={rowsPerPage}
onChange={onRowsPerPageChange}
id={selectId}
Expand Down
57 changes: 57 additions & 0 deletions packages/mui-material/src/TablePagination/TablePagination.test.js
View file
Open in desktop
Expand Up @@ -7,6 +7,9 @@ import TableFooter from '@mui/material/TableFooter';
import TableCell from '@mui/material/TableCell';
import TableRow from '@mui/material/TableRow';
import TablePagination, { tablePaginationClasses as classes } from '@mui/material/TablePagination';
import { inputClasses } from '@mui/material/Input';
import { outlinedInputClasses } from '@mui/material/OutlinedInput';
import { filledInputClasses } from '@mui/material/FilledInput';

describe('<TablePagination />', () => {
const noop = () => {};
Expand Down Expand Up @@ -399,6 +402,38 @@ describe('<TablePagination />', () => {
const [combobox] = getAllByRole('button');
expect(combobox).toHaveAccessibleName('Rows per page: 10');
});

['standard', 'outlined', 'filled'].forEach((variant) => {
it(`should be able to apply the ${variant} variant to select`, () => {
const { getAllByRole } = render(
<table>
<TableFooter>
<TableRow>
<TablePagination
count={1}
page={0}
onPageChange={noop}
onRowsPerPageChange={noop}
rowsPerPage={10}
SelectProps={{ variant }}
/>
</TableRow>
</TableFooter>
</table>,
);

const [combobox] = getAllByRole('button');
const comboboxContainer = combobox.parentElement;

if (variant === 'standard') {
expect(comboboxContainer).to.have.class(inputClasses.root);
} else if (variant === 'outlined') {
expect(comboboxContainer).to.have.class(outlinedInputClasses.root);
} else if (variant === 'filled') {
expect(comboboxContainer).to.have.class(filledInputClasses.root);
}
});
});
});

describe('prop: rowsPerPage', () => {
Expand Down Expand Up @@ -447,4 +482,26 @@ describe('<TablePagination />', () => {
);
});
});

it('should not have "variant" attribute on TablePaginationSelect', () => {
const { getAllByRole } = render(
<table>
<TableFooter>
<TableRow>
<TablePagination
count={1}
page={0}
onPageChange={noop}
onRowsPerPageChange={noop}
rowsPerPage={10}
/>
</TableRow>
</TableFooter>
</table>,
);

const [combobox] = getAllByRole('button');

expect(combobox.parentElement).not.to.have.attribute('variant');
});
});