Material-ui: TablePagination => onChangePage

[sajera]

Unknown reason TablePagination duplicate firing onChangePage without event

Example component

/**
 * Table pagination
 *
 * @param {Object} props
 * @public
 */
function Pagination ( { page, size, sizes, disabled, label, totalPages, changePage, changeSize } ) {
    return (<TableFooter>
        <TableRow>
            <TablePagination
                page={page}
                count={totalPages}
                rowsPerPage={size}
                rowsPerPageOptions={sizes}
                ActionsComponent={PageActions}
                labelDisplayedRows={PageLabels}
                // unknown reason material pagination duplicate firing function without event
                onChangePage={(e, page) => changePage(page)}
                labelRowsPerPage={label||(<strong> SIZE </strong>)}
                onChangeRowsPerPage={e => changeSize(e.target.value)}
                SelectProps={{ disabled, renderValue: value => (<strong>&nbsp;{value}&nbsp;&nbsp;</strong>) }}
                    />
        </TableRow>
    </TableFooter>);
}
/**
 * Pagination actions
 *
 * @param {Object} props
 * @public
 */
function PageActions ({ className, count, page, onChangePage, disabled }) {
    let disableNext = disabled || page >= count;
    let disablePrev = disabled || page === 0;
    return (<div className={className}>
        <Tooltip title="First Page" placement="bottom" enterDelay={300}><span>
            <IconButton onClick={e=>onChangePage(e, 0)} disabled={disablePrev} aria-label="First Page">
                <FirstPage />
            </IconButton>
        </span></Tooltip>
        <Tooltip title="Prev Page" placement="bottom" enterDelay={300}><span>
            <IconButton onClick={e=>onChangePage(e, page-1)} disabled={disablePrev} aria-label="Previous Page">
                <KeyboardArrowLeft />
            </IconButton>
        </span></Tooltip>
        <Tooltip title="Next Page" placement="bottom" enterDelay={300}><span>
            <IconButton onClick={e=>onChangePage(e, page+1)} disabled={disableNext} aria-label="Next Page">
                <KeyboardArrowRight />
            </IconButton>
        </span></Tooltip>
        <Tooltip title="Last Page" placement="bottom" enterDelay={300}><span>
            <IconButton onClick={e=>onChangePage(e, count-1)} disabled={disableNext} aria-label="Last Page">
                <LastPage />
            </IconButton>
        </span></Tooltip>
    </div>);
}

| Tech | Version |
|--------------|---------|
| Material-UI | ^3.4.0 |
| React | ^16.2.0 |
| Browser | Any |

[oliviertassinari]

@sajera Please provide a full reproduction test case. This would help a lot 👷 .
A live example would be perfect. This codesandbox.io template _may_ be a good starting point. Thank you!

Don't miss this logic:
https://github.com/mui-org/material-ui/blob/5f48bde637e36f98748f0bf182de96ac28871655/packages/material-ui/src/TablePagination/TablePagination.js#L72-L80

[ItsLeeOwen]

I found that ^ helper logic a bit troublesome in a scenario where my data sourcing the table was being cleared on new http request actions.

For anyone else finding their way to this thread, you can either check for an event in the handler, or ensure you're not resetting count and/or rowsPerPage.

private onChangePage = (event: any, page: number) => {
    const { onChangePage } = this.props
    if (event && "function" === typeof onChangePage) {
        // only handle user initiated onChangePage events
        onChangePage(page)
    }
}

[danielkolesnik]

@oliviertassinari, after a couple of hours, spent with console.log on the different levels of the saga, redux etc.
I've found that there is one redundant console.log from the component with _event==null_ and _page==0_ which actually ruins my business logic. So because of the impossibility of repeat event = null in normal situations I've decided to make the same check as @ItsLeeOwen. But to find the reason for crashes was very tricky...

changePage = (e, page) => { // NOTE: without this check, there is an issue with "material-ui" TablePagination component // which triggers changePage() with e = null, page = 0. if(e) { this.props.changePage(e, page); } };

[eps1lon]

@danielkolesnik Did you change the rowsPerPage or count without adjusting the page?. We currently trigger this event in case the current page is greater than the maximum number of pages given count and rowsPerPage.

I think we should add this to the documentation. It currently does not mention this case:
https://github.com/mui-org/material-ui/blob/ad4dae6fdd19d586166c0bd98343ae762794aac5/packages/material-ui/src/TablePagination/TablePagination.js#L74-L79

[danielkolesnik]

@eps1lon , yes, I store table meta information such as page and rows per page in URL query params and in the reducer, it gives me the possibility to share links to the concrete table page. That's why before requesting for content for the table to the server I check the query parameters, and if something wrong I change them to the standard one, after all, store them into the reducer(when it happens they passes into TablePagination properties). Finally, depending on these parameters, I request the data, which consists of the entities array(for current count and page) and total count (for TablePagination count property).
So firstly I set up page and rowsPerPage values, and after the response, I update count and entries of the table.
It was not really cool when after setting up page and rowsPerPage and sending the request to the server the component change page value by itself to zero which triggers one more time procedure of requesting data based on new query parameters...

[eps1lon]

@danielkolesnik I haven't really looked at the implication but we shouldn't sanitize the page prop in my opinion anyway.

It's not even obvious that it should trigger onPageChange with the last page. Maybe it should rather reset, maybe keep the relative previous positions. All of those scenarios should be handled by the user.

It's in general a good idea to keep a clear distinction between controlled and uncontrolled components. This one is mostly controlled with some uncontrolled aspect WRT to page.

I would rather add a prop type warning that the page is out of bounds.

[danielkolesnik]

@eps1lon, In any case, the decision to make you. I'm just happy that figures out, that it produces the fake event, so now I can prevent it and do my best with your package, which is actually one of the best, as for me. Thank you for your insights! 👍 🥇

[leMaik]

I looked in the original PR and noticed that the fake event has been in the pagination since the component was added and somehow, nobody complained so far. :sweat_smile:

@eps1lon @oliviertassinari How should we handle this case? Do _nothing_ or raise a warning in non-production environments? :thinking: Would this be considered a breaking change then?

[eps1lon]

I would just clamp the value i.e. limit it to 0..(count / rowsPerPage) and issue a warning via prop types if it is out of range.

Gets rid of some (opinionated) code and is more explicit about what's actually happening.

v4 is a good opportunity to introduce some breaking changes to make the code base smaller. In this case the only breaking change is that we don't emit an event but rather a warning.

[leMaik]

a warning via prop types if it is out of range

@eps1lon Is there a way can we warn about dynamic out-of-range errors via PropTypes?

[eps1lon]

a warning via prop types if it is out of range

@eps1lon Is there a way can we warn about dynamic out-of-range errors via PropTypes?

If all you need is props then you can write a custom validator. We use those for deprecation warnings amongs other things like: https://github.com/mui-org/material-ui/blob/8374f48052708f3e8d270e4e7608c242ca0918fd/packages/material-ui/src/ListItem/ListItem.js#L190-215

[leMaik]

@eps1lon Thank you, didn't know about custom validators. I'll prepare a PR that adds a warning later this day. :hourglass:

[KaRkY]

I have a question is out of range page really a warning? On first load of lets say page 4 it is always out of range because you can not posibly know how many records there are before data is loaded.

[eps1lon]

I have a question is out of range page really a warning? On first load of lets say page 4 it is always out of range because you can not posibly know how many records there are before data is loaded.

We can't know in the library if the props will be fixed in the future. You could just pass page={0} initially.