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

[TablePagination] Re-introduce deprecated onChangePage to ActionsComponent #27407

Merged
merged 3 commits into from
Jul 27, 2021
Merged

[TablePagination] Re-introduce deprecated onChangePage to ActionsComponent #27407

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
416742a
[TablePagination] Re-introduce deprecated `onChangePage` to `ActionsC…
eps1lon Jul 22, 2021
427907e
Update packages/material-ui/src/TablePagination/TablePaginationAction…
eps1lon Jul 23, 2021
4a7bc07
sloppy type fix
eps1lon Jul 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions packages/material-ui/src/TablePagination/TablePagination.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ const TablePagination = React.forwardRef(function TablePagination(props, ref) {
labelRowsPerPage = 'Rows per page:',
nextIconButtonProps,
nextIconButtonText = 'Next page',
onChangePage: onChangePageProp,
onPageChange: onPageChangeProp,
onChangePage,
onPageChange,
Copy link
Member

Choose a reason for hiding this comment

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

We should make this optional prop too, no? https://github.com/mui-org/material-ui/blob/416742a8fddf03cffba2a68c90b63662bfb5f814/packages/material-ui/src/TablePagination/TablePagination.d.ts#L35

Copy link
Member Author

Choose a reason for hiding this comment

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

Why is this relevant to prop destructuring?

Copy link
Member

Choose a reason for hiding this comment

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

What I meant is that, in order to fix the issue, we need to make both props optional in this component too, as we made them in TablePaginationActions. Currently onPageChange is required.

Copy link
Member Author

Choose a reason for hiding this comment

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

What I meant is that, in order to fix the issue

What issue are you referring to? The original issue is fixed and the test suite is green. You're probably not talking about the runtime which is confusing since the conversation is targetted at the .js file.

Copy link
Member

Choose a reason for hiding this comment

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

@mnajdova I would personally keep the types failing, to give a stronger signal to developers that they need to update their code. But no strong point of view.

Copy link
Member

Choose a reason for hiding this comment

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

Agree the runtime issue is fixed, I thought that it's good chance to fix the changed interface as well, as it is a breaking change for the typescript users. Here is the discussion around it: #23789 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand how the types are failing. I only have the issue and test suite to go off. If you think there's a problem, please provide a concrete example.

onChangeRowsPerPage: onChangeRowsPerPageProp,
onRowsPerPageChange: onRowsPerPageChangeProp,
page,
Expand All @@ -100,7 +100,6 @@ const TablePagination = React.forwardRef(function TablePagination(props, ref) {
...other
} = props;

const onChangePage = onChangePageProp || onPageChangeProp;
const onChangeRowsPerPage = onChangeRowsPerPageProp || onRowsPerPageChangeProp;
eps1lon marked this conversation as resolved.
Show resolved Hide resolved

let colSpan;
Expand Down Expand Up @@ -167,7 +166,8 @@ const TablePagination = React.forwardRef(function TablePagination(props, ref) {
'aria-label': nextIconButtonText,
...nextIconButtonProps,
}}
onPageChange={onChangePage}
onChangePage={onChangePage}
onPageChange={onPageChange}
page={page}
rowsPerPage={rowsPerPage}
/>
Expand Down
4 changes: 4 additions & 0 deletions packages/material-ui/src/TablePagination/TablePaginationActions.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ export interface TablePaginationActionsProps extends React.HTMLAttributes<HTMLDi
backIconButtonProps?: Partial<IconButtonProps>;
count: number;
nextIconButtonProps?: Partial<IconButtonProps>;
/**
* @deprecated Use onPageChange instead.
*/
onChangePage: (event: React.MouseEvent<HTMLButtonElement> | null, page: number) => void;
onPageChange: (event: React.MouseEvent<HTMLButtonElement> | null, page: number) => void;
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
page: number;
rowsPerPage: number;
Expand Down
14 changes: 12 additions & 2 deletions packages/material-ui/src/TablePagination/TablePaginationActions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ const TablePaginationActions = React.forwardRef(function TablePaginationActions(
backIconButtonProps,
count,
nextIconButtonProps,
onPageChange,
onChangePage = () => {},
onPageChange = () => {},
page,
rowsPerPage,
...other
Expand All @@ -22,10 +23,12 @@ const TablePaginationActions = React.forwardRef(function TablePaginationActions(
const theme = useTheme();

const handleBackButtonClick = (event) => {
onChangePage(event, page - 1);
onPageChange(event, page - 1);
};

const handleNextButtonClick = (event) => {
onChangePage(event, page + 1);
onPageChange(event, page + 1);
};

Expand Down Expand Up @@ -70,7 +73,14 @@ TablePaginationActions.propTypes = {
* @param {object} event The event source of the callback.
* @param {number} page The page selected.
*/
onPageChange: PropTypes.func.isRequired,
onChangePage: PropTypes.func,
/**
* Callback fired when the page is changed.
*
* @param {object} event The event source of the callback.
* @param {number} page The page selected.
*/
onPageChange: PropTypes.func,
/**
* The zero-based index of the current page.
*/
Expand Down