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.

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

[DataGrid] Reset sortedRows state on prop change #599

Merged
merged 13 commits into from
Nov 18, 2020
2 changes: 1 addition & 1 deletion packages/grid/_modules_/grid/GridComponent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ export const GridComponent = React.forwardRef<HTMLDivElement, GridComponentProps
useRows(props.rows, apiRef);
useKeyboard(rootContainerRef, apiRef);
useSelection(apiRef);
useSorting(apiRef);
useSorting(apiRef, props.rows);

useContainerProps(windowRef, apiRef);
const renderCtx = useVirtualRows(columnsHeaderRef, windowRef, renderingZoneRef, apiRef);
Expand Down
12 changes: 10 additions & 2 deletions packages/grid/_modules_/grid/hooks/features/sorting/useSorting.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { FeatureModeConstant } from '../../../models/featureMode';
import { CellParams } from '../../../models/params/cellParams';
import { ColParams } from '../../../models/params/colParams';
import { SortModelParams } from '../../../models/params/sortModelParams';
import { RowModel } from '../../../models/rows';
import { RowModel, RowsProp } from '../../../models/rows';
import { FieldComparatorList, SortItem, SortModel } from '../../../models/sortModel';
import { buildCellParams } from '../../../utils/paramsUtils';
import { isDesc, nextSortDirection } from '../../../utils/sortingUtils';
Expand All @@ -29,7 +29,7 @@ import { useGridState } from '../core/useGridState';
import { rowCountSelector, unorderedRowModelsSelector } from '../rows/rowsSelector';
import { SortingState } from './sortingState';

export const useSorting = (apiRef: ApiRef) => {
export const useSorting = (apiRef: ApiRef, rowsProp: RowsProp) => {
const logger = useLogger('useSorting');
const allowMultipleSorting = React.useRef<boolean>(false);
const comparatorList = React.useRef<FieldComparatorList>([]);
Expand Down Expand Up @@ -236,6 +236,14 @@ export const useSorting = (apiRef: ApiRef) => {
const sortApi: SortApi = { getSortModel, setSortModel, onSortModelChange, applySorting };
useApiMethod(apiRef, sortApi, 'SortApi');

React.useEffect(() => {
// When the rows prop change, we reset the sortedRows state.
setGridState((state) => ({
...state,
sorting: { ...state.sorting, sortedRows: rowsProp.map((row) => row.id) },
Copy link
Member

Choose a reason for hiding this comment

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

If we sorted the rows directly, would it solve #599 (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.

At least we fixed the reported bug and we have a solid test in place, so why not defer a better solution to later. I suspect it will require a refactoring of the state. There is an issue with the control flow somewhere, it could be imperative calls that shouldn't be here, it could be missing dependencies, a desynchronization, no idea :).

I will fix it ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

}));
}, [rowsProp, setGridState]);

React.useEffect(() => {
if (rowCount > 0 && options.sortingMode === FeatureModeConstant.client) {
logger.debug('row changed, applying sortModel');
Expand Down
62 changes: 61 additions & 1 deletion packages/grid/data-grid/src/DataGrid.test.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as React from 'react';
import PropTypes from 'prop-types';
// @ts-expect-error need to migrate helpers to TypeScript
import { createClientRender, ErrorBoundary } from 'test/utils';
import { createClientRender, fireEvent, screen, ErrorBoundary } from 'test/utils';
import { useFakeTimers } from 'sinon';
import { expect } from 'chai';
import { DataGrid } from '@material-ui/data-grid';
Expand Down Expand Up @@ -266,6 +266,66 @@ describe('<DataGrid />', () => {
expect(getColumnValues()).to.deep.equal(['Puma', 'Nike', 'Adidas'].reverse());
});
});

describe('sorting', () => {
it('should sort when clicking the header cell', () => {
render(
<div style={{ width: 300, height: 300 }}>
<DataGrid {...defaultProps} />
</div>,
);
const header = screen.getByRole('columnheader', { name: 'brand' });
expect(getColumnValues()).to.deep.equal(['Nike', 'Adidas', 'Puma']);
fireEvent.click(header);
expect(getColumnValues()).to.deep.equal(['Adidas', 'Nike', 'Puma']);
fireEvent.click(header);
expect(getColumnValues()).to.deep.equal(['Puma', 'Nike', 'Adidas']);
});

it('should keep rows sorted when rows prop change', () => {
interface TestCaseProps {
rows: any[];
}
const TestCase = (props: TestCaseProps) => {
const { rows } = props;
return (
<div style={{ width: 300, height: 300 }}>
<DataGrid
{...defaultProps}
rows={rows}
sortModel={[
{
field: 'brand',
sort: 'asc',
},
]}
/>
</div>
);
};

const { setProps } = render(<TestCase rows={defaultProps.rows} />);
expect(getColumnValues()).to.deep.equal(['Adidas', 'Nike', 'Puma']);

setProps({
rows: [
{
id: 3,
brand: 'Asics',
},
{
id: 4,
brand: 'RedBull',
},
{
id: 5,
brand: 'Hugo',
},
],
});
expect(getColumnValues()).to.deep.equal(['Asics', 'Hugo', 'RedBull']);
});
});
});

describe('warnings', () => {
Expand Down
16 changes: 0 additions & 16 deletions packages/grid/x-grid/src/XGrid.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -206,22 +206,6 @@ describe('<XGrid />', () => {
});
});

describe('sorting', () => {
it('should sort when clicking the header cell', () => {
render(
<div style={{ width: 300, height: 300 }}>
<XGrid {...defaultProps} />
</div>,
);
const header = screen.getByRole('columnheader', { name: 'brand' });
expect(getColumnValues()).to.deep.equal(['Nike', 'Adidas', 'Puma']);
fireEvent.click(header);
expect(getColumnValues()).to.deep.equal(['Adidas', 'Nike', 'Puma']);
fireEvent.click(header);
expect(getColumnValues()).to.deep.equal(['Puma', 'Nike', 'Adidas']);
});
});

describe('state', () => {
it('should trigger on state change and pass the correct params', () => {
let onStateParams;
Expand Down
58 changes: 58 additions & 0 deletions packages/storybook/src/stories/grid-sorting.stories.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import { Button } from '@material-ui/core';
import { randomInt } from '@material-ui/x-grid-data-generator';
import * as React from 'react';
import {
ColDef,
Expand All @@ -6,6 +8,7 @@ import {
SortModelParams,
SortModel,
useApiRef,
GridOverlay,
} from '@material-ui/x-grid';
import { withKnobs } from '@storybook/addon-knobs';
import { action } from '@storybook/addon-actions';
Expand Down Expand Up @@ -432,3 +435,58 @@ export const ServerSideSorting = () => {
</div>
);
};

export const ResetSortingRows = () => {
const columns = [
{
field: 'name',
width: 200,
},
{
field: 'team',
width: 200,
},
];
const [rows, setRows] = React.useState<RowsProp>([]);

const createRandomRows = () => {
const randomRows: any[] = [];

for (let i = 0; i < 10; i += 1) {
const id = randomInt(0, 100000).toString();
randomRows.push({ id, name: 'name test', team: id });
}

setRows(randomRows);
};

return (
<div
style={{
height: '1000px',
dtassone marked this conversation as resolved.
Show resolved Hide resolved
}}
>
<Button onClick={() => createRandomRows()}>Random Rows</Button>
<XGrid
rows={rows}
columns={columns}
loading={rows.length === 0}
rowHeight={56}
hideFooter
showCellRightBorder
showColumnRightBorder
disableExtendRowFullWidth
onColumnHeaderClick={() => {}}
dtassone marked this conversation as resolved.
Show resolved Hide resolved
sortModel={[
{
field: 'name',
sort: 'asc',
},
]}
components={{
noRowsOverlay: () => <GridOverlay>No Data</GridOverlay>,
}}
dtassone marked this conversation as resolved.
Show resolved Hide resolved
/>
</div>
);
};