From 3d75b8a1b83686d3bb1c0d000866aee69b8917d5 Mon Sep 17 00:00:00 2001 From: damien Date: Mon, 14 Sep 2020 14:25:58 +0200 Subject: [PATCH 1/6] fix #229 setPage not reliable --- .../src/hooks/features/usePagination.ts | 59 ++++++++++--------- .../src/stories/grid-pagination.stories.tsx | 29 +++++++++ 2 files changed, 60 insertions(+), 28 deletions(-) diff --git a/packages/grid/x-grid-modules/src/hooks/features/usePagination.ts b/packages/grid/x-grid-modules/src/hooks/features/usePagination.ts index 6cf0d4dcb1aa6..3493e27636b10 100644 --- a/packages/grid/x-grid-modules/src/hooks/features/usePagination.ts +++ b/packages/grid/x-grid-modules/src/hooks/features/usePagination.ts @@ -74,16 +74,21 @@ export const usePagination = ( const setPage = React.useCallback( (page: number) => { - page = stateRef.current.pageCount >= page ? page : stateRef.current.pageCount; - apiRef.current.renderPage( - stateRef.current.paginationMode === FeatureModeConstant.client ? page : 1, - ); - + let hasPageChanged = false; + if(stateRef.current.rowCount > 0) { + page = stateRef.current.pageCount >= page ? page : stateRef.current.pageCount; + apiRef.current.renderPage( + stateRef.current.paginationMode === FeatureModeConstant.client ? page : 1, + ); + hasPageChanged = true; + } const params: PageChangeParams = { ...stateRef.current, page, }; - apiRef.current.publishEvent(PAGE_CHANGED, params); + if(hasPageChanged) { + apiRef.current.publishEvent(PAGE_CHANGED, params); + } if (stateRef.current.page !== page) { updateState({ page }); } @@ -147,6 +152,17 @@ export const usePagination = ( } }, [setPageSize, logger, getAutoPageSize]); + useApiEventHandler(apiRef, PAGE_CHANGED, options.onPageChange); + useApiEventHandler(apiRef, PAGESIZE_CHANGED, options.onPageSizeChange); + + const onResize = React.useCallback(() => { + if (options.autoPageSize) { + resetAutopageSize(); + } + }, [options.autoPageSize, resetAutopageSize]); + + useApiEventHandler(apiRef, RESIZE, onResize); + React.useEffect(() => { stateRef.current = state; }, [state]); @@ -157,14 +173,6 @@ export const usePagination = ( } }, [apiRef, stateRef, apiRef.current?.isInitialised]); - React.useEffect(() => { - updateState({ paginationMode: options.paginationMode! }); - }, [options.paginationMode, updateState]); - - React.useEffect(() => { - setPage(options.page != null ? options.page : 1); - }, [options.page, setPage]); - React.useEffect(() => { const rowCount = options.rowCount == null ? rows.length : options.rowCount; if (rowCount !== state.rowCount) { @@ -172,9 +180,7 @@ export const usePagination = ( const newPageCount = getPageCount(state.pageSize, rowCount); updateState({ pageCount: newPageCount, rowCount }); - if (state.page > newPageCount) { - setPage(newPageCount); - } + setPage(state.page); } }, [ rows.length, @@ -187,6 +193,14 @@ export const usePagination = ( state.page, ]); + React.useEffect(() => { + updateState({ paginationMode: options.paginationMode! }); + }, [options.paginationMode, updateState]); + + React.useEffect(() => { + setPage(options.page != null ? options.page : 1); + }, [options.page, setPage]); + React.useEffect(() => { if ( !options.autoPageSize && @@ -203,17 +217,6 @@ export const usePagination = ( } }, [options.autoPageSize, resetAutopageSize, columns.visible.length]); - useApiEventHandler(apiRef, PAGE_CHANGED, options.onPageChange); - useApiEventHandler(apiRef, PAGESIZE_CHANGED, options.onPageSizeChange); - - const onResize = React.useCallback(() => { - if (options.autoPageSize) { - resetAutopageSize(); - } - }, [options.autoPageSize, resetAutopageSize]); - - useApiEventHandler(apiRef, RESIZE, onResize); - const paginationApi: PaginationApi = { setPageSize, setPage, diff --git a/packages/storybook/src/stories/grid-pagination.stories.tsx b/packages/storybook/src/stories/grid-pagination.stories.tsx index 76f0b9a136e52..2e690f5adfa80 100644 --- a/packages/storybook/src/stories/grid-pagination.stories.tsx +++ b/packages/storybook/src/stories/grid-pagination.stories.tsx @@ -264,3 +264,32 @@ export function ServerPaginationWithEventHandler() { ); } +export function Page2Prop() { + const data = useData(2000, 200); + + return ( +
+ action('pageChange')(p)} + + /> +
+ ); +} +export function Page2Api() { + const data = useData(2000, 200); + const apiRef = useApiRef(); + + React.useEffect(() => { + apiRef.current.setPage(2); + }, [apiRef]); + + return ( +
+ action('pageChange')(p)} + /> +
+ ); +} \ No newline at end of file From 93ba6d1bb24ca4f2b6dcc6e0528b5e54cbb0800d Mon Sep 17 00:00:00 2001 From: damien Date: Mon, 14 Sep 2020 15:04:11 +0200 Subject: [PATCH 2/6] fix #228 issue with first page trigger a pageChange event --- .../src/hooks/features/usePagination.ts | 4 +++- .../storybook/src/stories/grid-pagination.stories.tsx | 11 +++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/packages/grid/x-grid-modules/src/hooks/features/usePagination.ts b/packages/grid/x-grid-modules/src/hooks/features/usePagination.ts index 3493e27636b10..b72e0ddb5e958 100644 --- a/packages/grid/x-grid-modules/src/hooks/features/usePagination.ts +++ b/packages/grid/x-grid-modules/src/hooks/features/usePagination.ts @@ -61,6 +61,7 @@ export const usePagination = ( ), }; const stateRef = React.useRef(initialState); + const prevPageRef = React.useRef(1); const [state, dispatch] = React.useReducer(paginationReducer, initialState); const updateState = React.useCallback( @@ -86,8 +87,9 @@ export const usePagination = ( ...stateRef.current, page, }; - if(hasPageChanged) { + if(hasPageChanged && prevPageRef.current !== page) { apiRef.current.publishEvent(PAGE_CHANGED, params); + prevPageRef.current = page; } if (stateRef.current.page !== page) { updateState({ page }); diff --git a/packages/storybook/src/stories/grid-pagination.stories.tsx b/packages/storybook/src/stories/grid-pagination.stories.tsx index 2e690f5adfa80..4f0270cc3f6ef 100644 --- a/packages/storybook/src/stories/grid-pagination.stories.tsx +++ b/packages/storybook/src/stories/grid-pagination.stories.tsx @@ -264,6 +264,17 @@ export function ServerPaginationWithEventHandler() { ); } +export function Page1Prop() { + const data = useData(2000, 200); + + return ( +
+ action('pageChange')(p)} + /> +
+ ); +} export function Page2Prop() { const data = useData(2000, 200); From 6720c7db86b74b3e812e9031d75024033333b448 Mon Sep 17 00:00:00 2001 From: damien Date: Mon, 14 Sep 2020 15:57:42 +0200 Subject: [PATCH 3/6] Added test for #229 --- packages/grid/data-grid/src/DataGrid.test.tsx | 55 ++++++++++++++++++- .../src/hooks/features/usePagination.ts | 4 +- .../src/stories/grid-pagination.stories.tsx | 30 +++++++--- 3 files changed, 77 insertions(+), 12 deletions(-) diff --git a/packages/grid/data-grid/src/DataGrid.test.tsx b/packages/grid/data-grid/src/DataGrid.test.tsx index 519e402fac8d3..e50abb725751c 100644 --- a/packages/grid/data-grid/src/DataGrid.test.tsx +++ b/packages/grid/data-grid/src/DataGrid.test.tsx @@ -4,7 +4,7 @@ import PropTypes from 'prop-types'; // @ts-ignore import { createClientRender } from 'test/utils'; import { expect } from 'chai'; -import { DataGrid } from '@material-ui/data-grid'; +import { DataGrid, useApiRef } from '@material-ui/data-grid'; describe('', () => { const render = createClientRender(); @@ -60,6 +60,59 @@ describe('', () => { container.firstChild.firstChild.firstChild, ); }); + it('applies page prop correctly', () => { + const rows = [ + { + id: 0, + brand: 'Nike', + }, + { + id: 1, + brand: 'Addidas', + }, + { + id: 2, + brand: 'Puma', + }, + ]; + render( +
+ +
, + ); + const cell = document.querySelector(`.MuiDataGrid-row:first-child .MuiDataGrid-cell`)!; + expect(cell.textContent).to.equal('Addidas'); + }); + }); + it('applies setPage correctly', () => { + const rows = [ + { + id: 0, + brand: 'Nike', + }, + { + id: 1, + brand: 'Addidas', + }, + { + id: 2, + brand: 'Puma', + }, + ]; + const GridTest = () => { + const apiRef = useApiRef(); + React.useEffect(() => { + apiRef.current.setPage(2); + }); + return ( +
+ +
+ ); + }; + const { container } = render(); + const cell = container.querySelector(`.MuiDataGrid-row:first-child .MuiDataGrid-cell`)!; + expect(cell.textContent).to.equal('Addidas'); }); }); diff --git a/packages/grid/x-grid-modules/src/hooks/features/usePagination.ts b/packages/grid/x-grid-modules/src/hooks/features/usePagination.ts index b72e0ddb5e958..9f5f69c36c4ea 100644 --- a/packages/grid/x-grid-modules/src/hooks/features/usePagination.ts +++ b/packages/grid/x-grid-modules/src/hooks/features/usePagination.ts @@ -76,7 +76,7 @@ export const usePagination = ( const setPage = React.useCallback( (page: number) => { let hasPageChanged = false; - if(stateRef.current.rowCount > 0) { + if (stateRef.current.rowCount > 0) { page = stateRef.current.pageCount >= page ? page : stateRef.current.pageCount; apiRef.current.renderPage( stateRef.current.paginationMode === FeatureModeConstant.client ? page : 1, @@ -87,7 +87,7 @@ export const usePagination = ( ...stateRef.current, page, }; - if(hasPageChanged && prevPageRef.current !== page) { + if (hasPageChanged && prevPageRef.current !== page) { apiRef.current.publishEvent(PAGE_CHANGED, params); prevPageRef.current = page; } diff --git a/packages/storybook/src/stories/grid-pagination.stories.tsx b/packages/storybook/src/stories/grid-pagination.stories.tsx index 4f0270cc3f6ef..bc949627fa72a 100644 --- a/packages/storybook/src/stories/grid-pagination.stories.tsx +++ b/packages/storybook/src/stories/grid-pagination.stories.tsx @@ -269,8 +269,12 @@ export function Page1Prop() { return (
- action('pageChange')(p)} + action('pageChange')(p)} />
); @@ -280,9 +284,13 @@ export function Page2Prop() { return (
- action('pageChange')(p)} - + action('pageChange')(p)} />
); @@ -297,10 +305,14 @@ export function Page2Api() { return (
- action('pageChange')(p)} + action('pageChange')(p)} />
); -} \ No newline at end of file +} From e05c98f0c57162a22515cb9e3f48f2f951d35e02 Mon Sep 17 00:00:00 2001 From: damien tassone Date: Tue, 15 Sep 2020 11:15:16 +0200 Subject: [PATCH 4/6] Apply suggestions from code review Co-authored-by: Olivier Tassinari --- packages/grid/data-grid/src/DataGrid.test.tsx | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/packages/grid/data-grid/src/DataGrid.test.tsx b/packages/grid/data-grid/src/DataGrid.test.tsx index e50abb725751c..919d137879323 100644 --- a/packages/grid/data-grid/src/DataGrid.test.tsx +++ b/packages/grid/data-grid/src/DataGrid.test.tsx @@ -60,7 +60,8 @@ describe('', () => { container.firstChild.firstChild.firstChild, ); }); - it('applies page prop correctly', () => { + + it('should apply the page prop correctly', () => { const rows = [ { id: 0, @@ -80,11 +81,12 @@ describe('', () => { , ); - const cell = document.querySelector(`.MuiDataGrid-row:first-child .MuiDataGrid-cell`)!; - expect(cell.textContent).to.equal('Addidas'); + const cell = document.querySelector('[role="cell"][aria-colindex="0"]')!; + expect(cell).to.have.text('Addidas'); }); }); - it('applies setPage correctly', () => { + + it('should apply setPage correctly', () => { const rows = [ { id: 0, @@ -111,8 +113,8 @@ describe('', () => { ); }; const { container } = render(); - const cell = container.querySelector(`.MuiDataGrid-row:first-child .MuiDataGrid-cell`)!; - expect(cell.textContent).to.equal('Addidas'); + const cell = document.querySelector('[role="cell"][aria-colindex="0"]')!; + expect(cell).to.have.text('Addidas'); }); }); From dcab894593e4a81274c7f8a4f3af6d9b794dee0e Mon Sep 17 00:00:00 2001 From: damien Date: Tue, 15 Sep 2020 14:52:15 +0200 Subject: [PATCH 5/6] move api test to xgrid --- packages/grid/data-grid/src/DataGrid.test.tsx | 33 +------------------ packages/grid/x-grid/src/XGrid.test.tsx | 32 +++++++++++++++++- 2 files changed, 32 insertions(+), 33 deletions(-) diff --git a/packages/grid/data-grid/src/DataGrid.test.tsx b/packages/grid/data-grid/src/DataGrid.test.tsx index 919d137879323..eb7b774134136 100644 --- a/packages/grid/data-grid/src/DataGrid.test.tsx +++ b/packages/grid/data-grid/src/DataGrid.test.tsx @@ -4,7 +4,7 @@ import PropTypes from 'prop-types'; // @ts-ignore import { createClientRender } from 'test/utils'; import { expect } from 'chai'; -import { DataGrid, useApiRef } from '@material-ui/data-grid'; +import { DataGrid } from '@material-ui/data-grid'; describe('', () => { const render = createClientRender(); @@ -85,37 +85,6 @@ describe('', () => { expect(cell).to.have.text('Addidas'); }); }); - - it('should apply setPage correctly', () => { - const rows = [ - { - id: 0, - brand: 'Nike', - }, - { - id: 1, - brand: 'Addidas', - }, - { - id: 2, - brand: 'Puma', - }, - ]; - const GridTest = () => { - const apiRef = useApiRef(); - React.useEffect(() => { - apiRef.current.setPage(2); - }); - return ( -
- -
- ); - }; - const { container } = render(); - const cell = document.querySelector('[role="cell"][aria-colindex="0"]')!; - expect(cell).to.have.text('Addidas'); - }); }); describe('warnings', () => { diff --git a/packages/grid/x-grid/src/XGrid.test.tsx b/packages/grid/x-grid/src/XGrid.test.tsx index 15b4b6a3f93dd..cb07aab5e0f81 100644 --- a/packages/grid/x-grid/src/XGrid.test.tsx +++ b/packages/grid/x-grid/src/XGrid.test.tsx @@ -1,7 +1,7 @@ import * as React from 'react'; import { screen, createClientRender, act, fireEvent } from 'test/utils'; import { expect } from 'chai'; -import { XGrid } from '@material-ui/x-grid'; +import { XGrid, useApiRef } from '@material-ui/x-grid'; import { useData } from 'packages/storybook/src/hooks/useData'; function getActiveCell() { @@ -212,6 +212,36 @@ describe('', () => { expect(row).to.not.have.class('Mui-selected'); expect(checkbox).to.have.property('checked', false); }); + it('should apply setPage correctly', () => { + const rows = [ + { + id: 0, + brand: 'Nike', + }, + { + id: 1, + brand: 'Addidas', + }, + { + id: 2, + brand: 'Puma', + }, + ]; + const GridTest = () => { + const apiRef = useApiRef(); + React.useEffect(() => { + apiRef.current.setPage(2); + }); + return ( +
+ +
+ ); + }; + render(); + const cell = document.querySelector('[role="cell"][aria-colindex="0"]')!; + expect(cell).to.have.text('Addidas'); + }); }); describe('sorting', () => { From 08391c9ef1cc09420b46d126b924b7b44dc1ffd5 Mon Sep 17 00:00:00 2001 From: damien Date: Tue, 15 Sep 2020 15:48:18 +0200 Subject: [PATCH 6/6] fix prettier --- packages/grid/x-grid/src/XGrid.test.tsx | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/grid/x-grid/src/XGrid.test.tsx b/packages/grid/x-grid/src/XGrid.test.tsx index cb07aab5e0f81..29fae4d49884f 100644 --- a/packages/grid/x-grid/src/XGrid.test.tsx +++ b/packages/grid/x-grid/src/XGrid.test.tsx @@ -234,7 +234,13 @@ describe('', () => { }); return (
- +
); };