[Table] Add deprecation for renamed TablePagination props #23789
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mnajdova
added
component: table
mnajdova
commented
Nov 30, 2020
| * Deprecated. Will be removed in v5. Please use the onPageChange prop instead. | ||
| * @deprecated | ||
| */ | ||
| onChangePage?: (event: React.MouseEvent<HTMLButtonElement> | null, page: number) => void; |
There was a problem hiding this comment.
I've changed the old prop to not be required and made the new prop required. I guess this is the correct change.
There was a problem hiding this comment.
Although this can be considered breaking change 😕
|
@material-ui/core: parsed: +Infinity% , gzip: +Infinity% Details of bundle changes.Comparing: 02b273d...219db8e Details of page changes
|
oliviertassinari
changed the title
oliviertassinari
requested changes
Nov 30, 2020
packages/material-ui/src/TablePagination/TablePagination.test.js
Outdated
Show resolved
Hide resolved
Should I open PR towards that branch? |
I think so, I have changed the baseline branch |
oliviertassinari
approved these changes
Nov 30, 2020
mbrookes
previously requested changes
Dec 1, 2020
Co-authored-by: Matt <github@nospam.33m.co>
mbrookes
approved these changes
Dec 2, 2020
42 tasks
oliviertassinari
pushed a commit
that referenced
this pull request
Jan 24, 2021
oliviertassinari
pushed a commit
that referenced
this pull request
Jan 25, 2021
|
This is a breaking change (see marmelab/react-admin#6421). It was released in a minor release, which breaks exisiting apps. Could this be reverted, or applied in a backwards compatible way? |
@fzaninotto what exactly broke? Could you provide a minimal reproduction using Material-UI components? If you are referring to the changes done in |
|
You've changed the TablePagination props type, and this is a public component. It now requires a prop called onPageChange, which it didn't require before. So a call For a repro use case, see the attached react-admin issue. |
|
same here. onChangePage was optional, onPageChange is required. |
|
@ggascoigne The type change seems expected to me. We don't follow semver with the types (AFAIK). The initial point #23789 (comment) seems different, a JS runtime issue. @mnajdova it seems we forgot to forward the old |
|
Well I'm not going to argue that it isn't correct, just that I didn't expect that the interface to a public component to change in a breaking manner and not be called out. It was easy enough to fix once I found this PR. I will add that I love this library and really appreciate all of the many hours of hard work that you all put into it. |
2 tasks
|
How about we do this diff: index b027226200..98770320d0 100644
--- a/packages/material-ui/src/TablePagination/TablePagination.d.ts
+++ b/packages/material-ui/src/TablePagination/TablePagination.d.ts
@@ -31,7 +31,7 @@ export interface TablePaginationTypeMap<P, D extends React.ElementType> {
* @param {number} page The page selected.
* @deprecated Use the onPageChange prop instead.
*/
- onChangePage?: (event: React.MouseEvent<HTMLButtonElement> | null, page: number) => void;
+ onChangePage: (event: React.MouseEvent<HTMLButtonElement> | null, page: number) => void;
onPageChange: (event: React.MouseEvent<HTMLButtonElement> | null, page: number) => void;
/**
* Callback fired when the number of rows per page is changed.
diff --git a/packages/material-ui/src/TablePagination/TablePaginationActions.d.ts b/packages/material-ui/src/TablePagination/TablePaginationActions.d.ts
index 17d76f48ea..be49a53bab 100644
--- a/packages/material-ui/src/TablePagination/TablePaginationActions.d.ts
+++ b/packages/material-ui/src/TablePagination/TablePaginationActions.d.ts
@@ -5,6 +5,8 @@ export interface TablePaginationActionsProps extends React.HTMLAttributes<HTMLDi
backIconButtonProps?: Partial<IconButtonProps>;
count: number;
nextIconButtonProps?: Partial<IconButtonProps>;
+ // @deprecated Use the onPageChange prop instead.
+ onChangePage: (event: React.MouseEvent<HTMLButtonElement> | null, page: number) => void;
onPageChange: (event: React.MouseEvent<HTMLButtonElement> | null, page: number) => void;
page: number;
rowsPerPage: number;
diff --git a/packages/material-ui/src/TablePagination/TablePaginationActions.js b/packages/material-ui/src/TablePagination/TablePaginationActions.js
index bbe0a95f0e..b009219e76 100644
--- a/packages/material-ui/src/TablePagination/TablePaginationActions.js
+++ b/packages/material-ui/src/TablePagination/TablePaginationActions.js
@@ -13,7 +13,8 @@ const TablePaginationActions = React.forwardRef(function TablePaginationActions(
backIconButtonProps,
count,
nextIconButtonProps,
- onPageChange,
+ onPageChange: onPageChangeProp,
+ onChangePage: onChangePageProp,
page,
rowsPerPage,
...other
@@ -21,6 +22,8 @@ const TablePaginationActions = React.forwardRef(function TablePaginationActions(
const theme = useTheme();
+ const onPageChange = onPageChangeProp || onChangePageProp;
+
const handleBackButtonClick = (event) => {
onPageChange(event, page - 1);
};It should fix both the TypeScript regressions as well as the JS regression in the |
|
Wait, in #23789 (comment) we have |
|
|
I've moved the suggestion on the issue. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Deprecation for #22900