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.

Sign up for GitHub

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

Clean up pagination handling in Unified Data Table #4

Closed
wants to merge 2 commits into from
Closed

Clean up pagination handling in Unified Data Table #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc9a883
Clean up pagination handling in Unified Data Table
davismcphee Nov 27, 2024
2ecf1da
Remove console log
davismcphee Nov 27, 2024
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
83 changes: 26 additions & 57 deletions packages/kbn-unified-data-table/src/components/data_table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import {
EuiDataGridProps,
EuiHorizontalRule,
EuiDataGridToolBarVisibilityDisplaySelectorOptions,
EuiDataGridPaginationProps,
} from '@elastic/eui';
import type { DataView } from '@kbn/data-views-plugin/public';
import {
Expand Down Expand Up @@ -261,6 +262,12 @@ export interface UnifiedDataTableProps {
* Update rows per page state
*/
onUpdateRowsPerPage?: (rowsPerPage: number) => void;
/**
*
* this callback is triggered when user navigates to a different page
*
*/
onUpdatePageIndex?: (pageIndex: number) => void;
/**
* Configuration option to limit sample size slider
*/
Expand Down Expand Up @@ -420,12 +427,6 @@ export interface UnifiedDataTableProps {
* @param row
*/
getRowIndicator?: ColorIndicatorControlColumnParams['getRowIndicator'];
/**
*
* this callback is triggered when user navigates to a different page
*
*/
onChangePage?: (pageIndex: number) => void;
}

export const EuiDataGridMemoized = React.memo(EuiDataGrid);
Expand Down Expand Up @@ -470,6 +471,7 @@ export const UnifiedDataTable = ({
isPlainRecord = false,
rowsPerPageState,
onUpdateRowsPerPage,
onUpdatePageIndex,
onFieldEdited,
services,
renderCustomGridBody,
Expand Down Expand Up @@ -499,7 +501,6 @@ export const UnifiedDataTable = ({
getRowIndicator,
dataGridDensityState,
onUpdateDataGridDensity,
onChangePage: onChangePageProp,
}: UnifiedDataTableProps) => {
const { fieldFormats, toastNotifications, dataViewFieldEditor, uiSettings, storage, data } =
services;
Expand Down Expand Up @@ -603,74 +604,42 @@ export const UnifiedDataTable = ({
typeof rowsPerPageState === 'number' && rowsPerPageState > 0
? rowsPerPageState
: DEFAULT_ROWS_PER_PAGE;
const [pagination, setPagination] = useState({
pageIndex: 0,
pageSize: currentPageSize,
});
const [currentPageIndex, setCurrentPageIndex] = useState(0);
const rowCount = useMemo(() => (displayedRows ? displayedRows.length : 0), [displayedRows]);
const pageCount = useMemo(
() => Math.ceil(rowCount / pagination.pageSize),
[rowCount, pagination]
() => Math.ceil(rowCount / currentPageSize),
[currentPageSize, rowCount]
);

const paginationObj = useMemo(() => {
const pagination = useMemo<EuiDataGridPaginationProps | undefined>(() => {
const onChangeItemsPerPage = (pageSize: number) => {
onUpdateRowsPerPage?.(pageSize);
};

const onChangePage = (pageIndex: number) => {
setPagination((paginationData) => ({ ...paginationData, pageIndex }));
setCurrentPageIndex(pageIndex);
onUpdatePageIndex?.(pageIndex);
Comment on lines +620 to +621
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
setCurrentPageIndex(pageIndex);
onUpdatePageIndex?.(pageIndex);
const calculatedPageIndex = pageIndex > pageCount - 1 ? 0 : pageIndex
setCurrentPageIndex(calculatedPageIndex);
onUpdatePageIndex?.(calculatedPageIndex);

I think this implementation could be updated to fix the bug you found by calculating the page index within this callback...

Copy link
Owner

@logeekal logeekal Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@davismcphee , i do not think calculating there is needed, because onChangePage is not called when we need new pageIndex to be calculated. The other useEffect should be enough and i have included that.

Copy link
Owner

Choose a reason for hiding this comment

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

Also, I think onUpdatePageIndex should now be solely based on currentPageIndex. Any changes to that should be automatically trigger onUpdatePageIndex instead of calling onUpdatePageIndex at multiple places.

};

return isPaginationEnabled
? {
onChangeItemsPerPage,
onChangePage,
pageIndex: pagination.pageIndex > pageCount - 1 ? 0 : pagination.pageIndex,
pageSize: pagination.pageSize,
pageSizeOptions: rowsPerPageOptions ?? getRowsPerPageOptions(pagination.pageSize),
pageIndex: currentPageIndex > pageCount - 1 ? 0 : currentPageIndex,
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
pageIndex: currentPageIndex > pageCount - 1 ? 0 : currentPageIndex,
pageIndex: currentPageIndex,

...Then passing currentPageIndex directly here.

And we'd only need to manually sync when pageCount changes in a separate effect:

useEffect(() => {
  setCurrentPageIndex((previousPageIndex) => {
    const calculatedPageIndex = previousPageIndex > pageCount - 1 ? 0 : previousPageIndex;

    if (calculatedPageIndex !== previousPageIndex) {
      onUpdatePageIndex?.(calculatedPageIndex);
    }

    return calculatedPageIndex;
  });
}, [onUpdatePageIndex, pageCount]);

pageSize: currentPageSize,
pageSizeOptions: rowsPerPageOptions ?? getRowsPerPageOptions(currentPageSize),
}
: undefined;
}, [
currentPageIndex,
currentPageSize,
isPaginationEnabled,
pagination.pageIndex,
pagination.pageSize,
onUpdatePageIndex,
onUpdateRowsPerPage,
pageCount,
rowsPerPageOptions,
onUpdateRowsPerPage,
]);

useEffect(() => {
/*
* Only for pageSize
* Sync pageSize with consumer provided pageSize
*/
setPagination((paginationData) =>
paginationData.pageSize === currentPageSize
? paginationData
: { ...paginationData, pageSize: currentPageSize }
);
}, [currentPageSize]);

useEffect(() => {
/*
* Only for pageIndex
* Sync pagination with EUI calculated pageIndex
*
*/
setPagination((prevPagination) => ({
...prevPagination,
pageIndex: paginationObj?.pageIndex ?? 0,
}));
}, [paginationObj?.pageIndex]);

useEffect(() => {
/*
* Propagate new pageIndex to the consumer
*/
onChangePageProp?.(pagination.pageIndex);
}, [pagination.pageIndex, onChangePageProp]);

const unifiedDataTableContextValue = useMemo<DataTableContext>(
() => ({
expanded: expandedDoc,
Expand All @@ -683,8 +652,8 @@ export const UnifiedDataTable = ({
valueToStringConverter,
componentsTourSteps,
isPlainRecord,
pageIndex: isPaginationEnabled ? paginationObj?.pageIndex : 0,
pageSize: isPaginationEnabled ? paginationObj?.pageSize : displayedRows.length,
pageIndex: isPaginationEnabled ? pagination?.pageIndex : 0,
pageSize: isPaginationEnabled ? pagination?.pageSize : displayedRows.length,
}),
[
componentsTourSteps,
Expand All @@ -697,7 +666,7 @@ export const UnifiedDataTable = ({
onFilter,
setExpandedDoc,
selectedDocsState,
paginationObj,
pagination,
valueToStringConverter,
]
);
Expand Down Expand Up @@ -1181,7 +1150,7 @@ export const UnifiedDataTable = ({
data-test-subj="docTable"
leadingControlColumns={leadingControlColumns}
onColumnResize={onResize}
pagination={paginationObj}
pagination={pagination}
renderCellValue={renderCellValue}
ref={dataGridRef}
rowCount={rowCount}
Expand Down Expand Up @@ -1210,7 +1179,7 @@ export const UnifiedDataTable = ({
rowCount={rowCount}
sampleSize={sampleSizeState}
pageCount={pageCount}
pageIndex={paginationObj?.pageIndex}
pageIndex={pagination?.pageIndex}
totalHits={totalHits}
onFetchMoreRecords={onFetchMoreRecords}
data={data}
Expand Down
6 changes: 3 additions & 3 deletions ...ity_solution/public/timelines/components/timeline/unified_components/data_table/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ type CommonDataTableProps = {
| 'renderCustomGridBody'
| 'trailingControlColumns'
| 'isSortEnabled'
| 'onChangePage'
| 'onUpdatePageIndex'
>;

interface DataTableProps extends CommonDataTableProps {
Expand Down Expand Up @@ -110,7 +110,7 @@ export const TimelineDataTableComponent: React.FC<DataTableProps> = memo(
onSort,
onFilter,
leadingControlColumns,
onChangePage,
onUpdatePageIndex,
}) {
const dispatch = useDispatch();

Expand Down Expand Up @@ -426,7 +426,7 @@ export const TimelineDataTableComponent: React.FC<DataTableProps> = memo(
renderCustomGridBody={finalRenderCustomBodyCallback}
trailingControlColumns={finalTrailControlColumns}
externalControlColumns={leadingControlColumns}
onChangePage={onChangePage}
onUpdatePageIndex={onUpdatePageIndex}
/>
</StyledTimelineUnifiedDataTable>
</StatefulEventContext.Provider>
Expand Down
4 changes: 2 additions & 2 deletions ...ugins/security_solution/public/timelines/components/timeline/unified_components/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ interface Props {
dataView: DataView;
trailingControlColumns?: EuiDataGridProps['trailingControlColumns'];
leadingControlColumns?: EuiDataGridProps['leadingControlColumns'];
onChangePage?: UnifiedDataTableProps['onChangePage'];
onChangePage?: UnifiedDataTableProps['onUpdatePageIndex'];
}

const UnifiedTimelineComponent: React.FC<Props> = ({
Expand Down Expand Up @@ -444,7 +444,7 @@ const UnifiedTimelineComponent: React.FC<Props> = ({
onFilter={onAddFilter as DocViewFilterFn}
trailingControlColumns={trailingControlColumns}
leadingControlColumns={leadingControlColumns}
onChangePage={onChangePage}
onUpdatePageIndex={onChangePage}
/>
</EventDetailsWidthProvider>
</DropOverlayWrapper>
Expand Down