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] Do not apply filters on rowExpansionChange #8671

Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
0cee1cd
do not apply filters on rowExpansionChange
cherniavskii Apr 19, 2023
0c9f390
Merge branch 'master' into do-not-apply-filters-on-rowExpansionChange
cherniavskii Apr 20, 2023
b34e757
add unit tests
cherniavskii Apr 20, 2023
8e9ced1
Merge branch 'master' into do-not-apply-filters-on-rowExpansionChange
cherniavskii Apr 25, 2023
1e1e912
move useGridVisibleRowsLookup outside of useGridFilter
cherniavskii Apr 25, 2023
952c8a4
change shape of visibleRowsLookup state to visibleRows.lookup
cherniavskii Apr 25, 2023
db01474
rename useGridVisibleRowsLookup to useGridVisibleRowsState
cherniavskii Apr 25, 2023
b9cbd3d
Merge branch 'master' into do-not-apply-filters-on-rowExpansionChange
cherniavskii May 22, 2023
d055c1d
avoid useCallback for pure functions
cherniavskii May 22, 2023
0b1f969
Merge branch 'master' into do-not-apply-filters-on-rowExpansionChange
cherniavskii May 29, 2023
7422d23
move visible state management to filtering hook
cherniavskii May 29, 2023
c58488b
initialize visibleRows state along with filter state
cherniavskii May 30, 2023
baab0df
move visibleRows interfaces back to filtering
cherniavskii May 30, 2023
5b5cd60
move visible rows selectors to filter selectors
cherniavskii May 30, 2023
daa6bd8
avoid additional package exports
cherniavskii May 30, 2023
1aca18c
flatten visibleRows lookup state
cherniavskii May 31, 2023
a10b77e
rename visibleRows strategy processor
cherniavskii May 31, 2023
6bd5c21
fix tests
cherniavskii May 31, 2023
9e99470
Merge branch 'master' into do-not-apply-filters-on-rowExpansionChange
cherniavskii Jun 1, 2023
f0629a3
use type instead of interface
cherniavskii Jun 1, 2023
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
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ import {
columnGroupsStateInitializer,
MBilalShafi marked this conversation as resolved.
Show resolved Hide resolved
useGridLazyLoader,
useGridLazyLoaderPreProcessors,
useGridVisibleRowsState,
visibleRowsStateInitializer,
headerFilteringStateInitializer,
useGridHeaderFiltering,
} from '@mui/x-data-grid-pro/internals';
Expand Down Expand Up @@ -121,6 +123,7 @@ export const useDataGridPremiumComponent = (
useGridInitializeState(columnsStateInitializer, privateApiRef, props);
useGridInitializeState(rowPinningStateInitializer, privateApiRef, props);
useGridInitializeState(rowsStateInitializer, privateApiRef, props);
useGridInitializeState(visibleRowsStateInitializer, privateApiRef, props);
useGridInitializeState(editingStateInitializer, privateApiRef, props);
useGridInitializeState(focusStateInitializer, privateApiRef, props);
useGridInitializeState(sortingStateInitializer, privateApiRef, props);
Expand All @@ -145,6 +148,7 @@ export const useDataGridPremiumComponent = (
useGridRowPinning(privateApiRef, props);
useGridColumns(privateApiRef, props);
useGridRows(privateApiRef, props);
useGridVisibleRowsState(privateApiRef);
useGridParamsApi(privateApiRef);
useGridDetailPanel(privateApiRef, props);
useGridColumnSpanning(privateApiRef);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,6 @@ export const filterRowTreeFromGroupingColumns = (
params: FilterRowTreeFromTreeDataParams,
MBilalShafi marked this conversation as resolved.
Show resolved Hide resolved
): Omit<GridFilterState, 'filterModel'> => {
const { rowTree, isRowMatchingFilters, filterModel } = params;
const visibleRowsLookup: Record<GridRowId, boolean> = {};
const filteredRowsLookup: Record<GridRowId, boolean> = {};
const filteredDescendantCountLookup: Record<GridRowId, number> = {};

Expand Down Expand Up @@ -138,15 +137,8 @@ export const filterRowTreeFromGroupingColumns = (
}
}

visibleRowsLookup[node.id] = isPassingFiltering && areAncestorsExpanded;
filteredRowsLookup[node.id] = isPassingFiltering;

// TODO rows v6: Should we keep storing the visibility status of footer independently or rely on the group visibility in the selector ?
if (node.type === 'group' && node.footerId != null) {
visibleRowsLookup[node.footerId] =
isPassingFiltering && areAncestorsExpanded && !!node.childrenExpanded;
}

if (!isPassingFiltering) {
return 0;
}
Expand All @@ -169,7 +161,6 @@ export const filterRowTreeFromGroupingColumns = (
}

return {
visibleRowsLookup,
filteredRowsLookup,
filteredDescendantCountLookup,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
createRowTree,
Copy link
Member Author

Choose a reason for hiding this comment

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

@MBilalShafi

On the codesandbox where the package for this PR is installed, I played around and I observed that the page was not anymore stuck on row expansion change, but when I tried to change an aggregation function, it blocked the UI thread for a similar amount of time:

I think the reason is that our diffing algorithm is not smart enough to check, "ok, so out of 100 columns, the aggregation functions for column 1 has changed, let's go ahead and only re-compute the aggregation lookup for this column", rather it recomputes all the data once any column updates it's aggregation method.

That's a very good point, I'm sure we can optimize aggregation (especially when there's a lot of aggregated columns) 👍

updateRowTree,
RowTreeBuilderGroupingCriterion,
getVisibleRowsLookup,
} from '@mui/x-data-grid-pro/internals';
import { DataGridPremiumProcessedProps } from '../../../models/dataGridPremiumProps';
import {
Expand Down Expand Up @@ -224,6 +225,12 @@ export const useGridRowGroupingPreProcessors = (
[apiRef],
);

const getVisibleRows = React.useCallback<GridStrategyProcessor<'visibleRows'>>((params) => {
return {
lookup: getVisibleRowsLookup(params),
};
}, []);

useGridRegisterPipeProcessor(apiRef, 'hydrateColumns', updateGroupingColumn);
useGridRegisterStrategyProcessor(
apiRef,
Expand All @@ -233,6 +240,7 @@ export const useGridRowGroupingPreProcessors = (
);
useGridRegisterStrategyProcessor(apiRef, ROW_GROUPING_STRATEGY, 'filtering', filterRows);
useGridRegisterStrategyProcessor(apiRef, ROW_GROUPING_STRATEGY, 'sorting', sortRows);
useGridRegisterStrategyProcessor(apiRef, ROW_GROUPING_STRATEGY, 'visibleRows', getVisibleRows);

/**
* 1ST RENDER
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2459,6 +2459,39 @@ describe('<DataGridPremium /> - Row Grouping', () => {
expect(getColumnValues(1)).to.deep.equal(['', 'Cat 1 (1)', '', 'Cat 2 (2)', '', '']);
});
});

it('should not apply filters when the row is expanded', () => {
render(
<Test
initialState={{
rowGrouping: { model: ['category1'] },
}}
/>,
);

const onFilteredRowsSet = spy();
apiRef.current.subscribeEvent('filteredRowsSet', onFilteredRowsSet);

fireEvent.click(getCell(0, 0).querySelector('button')!);
expect(onFilteredRowsSet.callCount).to.equal(0);
});

it('should not apply filters when the row is collapsed', () => {
render(
<Test
initialState={{
rowGrouping: { model: ['category1'] },
}}
defaultGroupingExpansionDepth={-1}
/>,
);

const onFilteredRowsSet = spy();
apiRef.current.subscribeEvent('filteredRowsSet', onFilteredRowsSet);

fireEvent.click(getCell(0, 0).querySelector('button')!);
expect(onFilteredRowsSet.callCount).to.equal(0);
});
});

describe('apiRef: addRowGroupingCriteria', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ import {
rowSelectionStateInitializer,
useGridColumnGrouping,
columnGroupsStateInitializer,
useGridVisibleRowsState,
visibleRowsStateInitializer,
headerFilteringStateInitializer,
useGridHeaderFiltering,
} from '@mui/x-data-grid/internals';
Expand Down Expand Up @@ -107,6 +109,7 @@ export const useDataGridProComponent = (
useGridInitializeState(columnsStateInitializer, apiRef, props);
useGridInitializeState(rowPinningStateInitializer, apiRef, props);
useGridInitializeState(rowsStateInitializer, apiRef, props);
useGridInitializeState(visibleRowsStateInitializer, apiRef, props);
useGridInitializeState(editingStateInitializer, apiRef, props);
useGridInitializeState(focusStateInitializer, apiRef, props);
useGridInitializeState(sortingStateInitializer, apiRef, props);
Expand All @@ -128,6 +131,7 @@ export const useDataGridProComponent = (
useGridRowPinning(apiRef, props);
useGridColumns(apiRef, props);
useGridRows(apiRef, props);
useGridVisibleRowsState(apiRef);
useGridParamsApi(apiRef);
useGridDetailPanel(apiRef, props);
useGridColumnSpanning(apiRef);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ export const filterRowTreeFromTreeData = (
params: FilterRowTreeFromTreeDataParams,
): Omit<GridFilterState, 'filterModel'> => {
const { rowTree, disableChildrenFiltering, isRowMatchingFilters } = params;
const visibleRowsLookup: Record<GridRowId, boolean> = {};
const filteredRowsLookup: Record<GridRowId, boolean> = {};
const filteredDescendantCountLookup: Record<GridRowId, number> = {};

Expand Down Expand Up @@ -86,15 +85,8 @@ export const filterRowTreeFromTreeData = (
}
}

visibleRowsLookup[node.id] = shouldPassFilters && areAncestorsExpanded;
filteredRowsLookup[node.id] = shouldPassFilters;

// TODO: Should we keep storing the visibility status of footer independently or rely on the group visibility in the selector ?
if (node.type === 'group' && node.footerId != null) {
visibleRowsLookup[node.footerId] =
shouldPassFilters && areAncestorsExpanded && !!node.childrenExpanded;
}

if (!shouldPassFilters) {
return 0;
}
Expand All @@ -117,7 +109,6 @@ export const filterRowTreeFromTreeData = (
}

return {
visibleRowsLookup,
filteredRowsLookup,
filteredDescendantCountLookup,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
} from '../../../utils/tree/models';
import { sortRowTree } from '../../../utils/tree/sortRowTree';
import { updateRowTree } from '../../../utils/tree/updateRowTree';
import { getVisibleRowsLookup } from '../../../utils/tree/utils';

export const useGridTreeDataPreProcessors = (
privateApiRef: React.MutableRefObject<GridPrivateApiPro>,
Expand Down Expand Up @@ -203,6 +204,12 @@ export const useGridTreeDataPreProcessors = (
[privateApiRef, props.disableChildrenSorting],
);

const getVisibleRows = React.useCallback<GridStrategyProcessor<'visibleRows'>>((params) => {
return {
lookup: getVisibleRowsLookup(params),
};
}, []);

useGridRegisterPipeProcessor(privateApiRef, 'hydrateColumns', updateGroupingColumn);
useGridRegisterStrategyProcessor(
privateApiRef,
Expand All @@ -212,6 +219,12 @@ export const useGridTreeDataPreProcessors = (
);
useGridRegisterStrategyProcessor(privateApiRef, TREE_DATA_STRATEGY, 'filtering', filterRows);
useGridRegisterStrategyProcessor(privateApiRef, TREE_DATA_STRATEGY, 'sorting', sortRows);
useGridRegisterStrategyProcessor(
privateApiRef,
TREE_DATA_STRATEGY,
'visibleRows',
MBilalShafi marked this conversation as resolved.
Show resolved Hide resolved
getVisibleRows,
);

/**
* 1ST RENDER
Expand Down
2 changes: 1 addition & 1 deletion packages/grid/x-data-grid-pro/src/internals/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,5 +51,5 @@ export type {
export { createRowTree } from '../utils/tree/createRowTree';
export { updateRowTree } from '../utils/tree/updateRowTree';
export { sortRowTree } from '../utils/tree/sortRowTree';
export { insertNodeInTree, removeNodeFromTree } from '../utils/tree/utils';
export { insertNodeInTree, removeNodeFromTree, getVisibleRowsLookup } from '../utils/tree/utils';
export type { RowTreeBuilderGroupingCriterion } from '../utils/tree/models';
44 changes: 44 additions & 0 deletions packages/grid/x-data-grid-pro/src/utils/tree/utils.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import {
GRID_ROOT_GROUP_ID,
GridChildrenFromPathLookup,
GridFilterState,
GridGroupNode,
GridLeafNode,
GridRowId,
GridRowTreeConfig,
GridRowsState,
GridTreeNode,
} from '@mui/x-data-grid';
import {
Expand Down Expand Up @@ -235,3 +237,45 @@ export const createUpdatedGroupsManager = (): GridRowTreeUpdatedGroupsManager =>
this.value[groupId][action] = true;
},
});

export const getVisibleRowsLookup = ({
tree,
filteredRowsLookup,
}: {
tree: GridRowsState['tree'];
filteredRowsLookup: GridFilterState['filteredRowsLookup'];
}) => {
if (!filteredRowsLookup) {
return {};
}

const visibleRowsLookup: Record<GridRowId, boolean> = {};

const handleTreeNode = (node: GridTreeNode, areAncestorsExpanded: boolean) => {
const isPassingFiltering = filteredRowsLookup[node.id];

if (node.type === 'group') {
node.children.forEach((childId) => {
const childNode = tree[childId];
handleTreeNode(childNode, areAncestorsExpanded && !!node.childrenExpanded);
});
}

visibleRowsLookup[node.id] = isPassingFiltering && areAncestorsExpanded;

// TODO rows v6: Should we keep storing the visibility status of footer independently or rely on the group visibility in the selector ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have any opinions on this comment? Are footers sometimes hidden?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you can have aggregation footer for each row group - see https://mui.com/x/react-data-grid/aggregation/#AggregationGetAggregationPosition.tsx

When the group is filtered out, the aggregation footer should be hidden as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIUC, the visibility of the group is always equal to the visibility of the footer, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not exactly, you can have a visible row group, but if it's not expanded - the footer row shouldn't be visible:

visibleRowsLookup[node.footerId] =
isPassingFiltering && areAncestorsExpanded && !!node.childrenExpanded;

row-group-footer.mov

if (node.type === 'group' && node.footerId != null) {
visibleRowsLookup[node.footerId] =
isPassingFiltering && areAncestorsExpanded && !!node.childrenExpanded;
}
};

const nodes = Object.values(tree);
for (let i = 0; i < nodes.length; i += 1) {
const node = nodes[i];
if (node.depth === 0) {
handleTreeNode(node, true);
}
}
return visibleRowsLookup;
};
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ import { useGridScroll } from '../hooks/features/scroll/useGridScroll';
import { useGridEvents } from '../hooks/features/events/useGridEvents';
import { useGridDimensions } from '../hooks/features/dimensions/useGridDimensions';
import { rowsMetaStateInitializer, useGridRowsMeta } from '../hooks/features/rows/useGridRowsMeta';
import {
useGridVisibleRowsState,
visibleRowsStateInitializer,
} from '../hooks/features/rows/useGridVisibleRowsState';
import { useGridStatePersistence } from '../hooks/features/statePersistence/useGridStatePersistence';
import { useGridColumnSpanning } from '../hooks/features/columns/useGridColumnSpanning';
import {
Expand Down Expand Up @@ -64,6 +68,7 @@ export const useDataGridComponent = (
useGridInitializeState(rowSelectionStateInitializer, privateApiRef, props);
useGridInitializeState(columnsStateInitializer, privateApiRef, props);
useGridInitializeState(rowsStateInitializer, privateApiRef, props);
useGridInitializeState(visibleRowsStateInitializer, privateApiRef, props);
useGridInitializeState(editingStateInitializer, privateApiRef, props);
useGridInitializeState(focusStateInitializer, privateApiRef, props);
useGridInitializeState(sortingStateInitializer, privateApiRef, props);
Expand All @@ -79,6 +84,7 @@ export const useDataGridComponent = (
useGridRowSelection(privateApiRef, props);
useGridColumns(privateApiRef, props);
useGridRows(privateApiRef, props);
useGridVisibleRowsState(privateApiRef);
useGridParamsApi(privateApiRef);
useGridColumnSpanning(privateApiRef);
useGridColumnGrouping(privateApiRef, props);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,18 @@
import {
GridRowTreeCreationParams,
GridRowTreeCreationValue,
GridRowsState,
} from '../../features/rows/gridRowsInterfaces';
import {
GridFilteringMethodParams,
GridFilteringMethodValue,
GridFilterState,
} from '../../features/filter/gridFilterState';
import {
GridSortingMethodParams,
GridSortingMethodValue,
} from '../../features/sorting/gridSortingState';
import type { GridVisibleRowsMethodValue } from '../../features/rows/gridVisibleRowsInterfaces';

export type GridStrategyProcessorName = keyof GridStrategyProcessingLookup;

Expand All @@ -32,6 +35,14 @@ export interface GridStrategyProcessingLookup {
params: GridSortingMethodParams;
value: GridSortingMethodValue;
};
visibleRows: {
group: 'rowTree';
params: {
tree: GridRowsState['tree'];
filteredRowsLookup: GridFilterState['filteredRowsLookup'];
};
value: GridVisibleRowsMethodValue;
};
}

export type GridStrategyProcessor<P extends GridStrategyProcessorName> = (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export const GRID_STRATEGIES_PROCESSORS: {
rowTreeCreation: 'rowTree',
filtering: 'rowTree',
sorting: 'rowTree',
visibleRows: 'rowTree',
};

type UntypedStrategyProcessors = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { GridStateCommunity } from '../../../models/gridStateCommunity';
import { gridSortedRowEntriesSelector } from '../sorting/gridSortingSelector';
import { gridColumnLookupSelector } from '../columns/gridColumnsSelector';
import { gridRowMaximumTreeDepthSelector, gridRowTreeSelector } from '../rows/gridRowsSelector';
import { gridVisibleRowsLookupSelector } from '../rows/gridVisibleRowsSelector';

/**
* @category Filtering
Expand All @@ -28,15 +29,6 @@ export const gridQuickFilterValuesSelector = createSelector(
(filterModel) => filterModel.quickFilterValues,
);

/**
* @category Filtering
* @ignore - do not document.
*/
export const gridVisibleRowsLookupSelector = createSelector(
gridFilterStateSelector,
(filterState) => filterState.visibleRowsLookup,
);

/**
* @category Filtering
* @ignore - do not document.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,6 @@ export interface GridFilterState {
* This is the equivalent of the `visibleRowsLookup` if all the groups were expanded.
*/
filteredRowsLookup: Record<GridRowId, boolean>;
/**
* Visibility status for each row.
* A row is visible if it is passing the filters AND if its parents are expanded.
* If a row is not registered in this lookup, it is visible.
*/
visibleRowsLookup: Record<GridRowId, boolean>;
/**
* Amount of descendants that are passing the filters.
* For the Tree Data, it includes all the intermediate depth levels (= amount of children + amount of grand children + ...).
Expand Down
Loading