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

[Discover] Data Grid Pagination Options #5610

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Workspace] Setup workspace skeleton and implement basic CRUD API ([#5075](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5075))
- [Decouple] Add new cross compatibility check core service which export functionality for plugins to verify if their OpenSearch plugin counterpart is installed on the cluster or has incompatible version to configure the plugin behavior([#4710](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4710))
- [Discover] Display inner properties in the left navigation bar [#5429](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5429)
- [Discover] Added customizable pagination options based on Discover UI settings [#5610](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5610)

### 🐛 Bug Fixes

Expand Down Expand Up @@ -947,4 +948,4 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)

### 🔩 Tests

- Update caniuse to fix failed integration tests ([#2322](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2322))
- Update caniuse to fix failed integration tests ([#2322](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2322))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know what changed here!

Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ import { DocViewFilterFn, OpenSearchSearchHit } from '../../doc_views/doc_views_
import { usePagination } from '../utils/use_pagination';
import { SortOrder } from '../../../saved_searches/types';
import { buildColumns } from '../../utils/columns';
import { useOpenSearchDashboards } from '../../../../../opensearch_dashboards_react/public';
import { DiscoverServices } from '../../../build_services';

export interface DataGridTableProps {
columns: string[];
Expand Down Expand Up @@ -52,9 +54,11 @@ export const DataGridTable = ({
isContextView = false,
isLoading = false,
}: DataGridTableProps) => {
const { services } = useOpenSearchDashboards<DiscoverServices>();

const [inspectedHit, setInspectedHit] = useState<OpenSearchSearchHit | undefined>();
const rowCount = useMemo(() => (rows ? rows.length : 0), [rows]);
const pagination = usePagination(rowCount);
const pagination = usePagination({ rowCount, services });

let adjustedColumns = buildColumns(columns);
// handle case where the user removes selected filed and leaves only time column
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { defaultPageOptions, generatePageSizeOptions } from './page_size_options';

describe('generatePageSizeOptions', () => {
it('generates default options and additional options based on sample size', () => {
const sampleSize = 1000;

const pageSizeOptions = generatePageSizeOptions(sampleSize);

// Expected result based on the provided sample size
const expectedOptions = [...defaultPageOptions, 500, 1000];

// Check if the generated options match the expected result
expect(pageSizeOptions).toEqual(expectedOptions);
});

it('handles edge case when sample size is less than maxSize', () => {
const sampleSize = 50;

// Call the function
const pageSizeOptions = generatePageSizeOptions(sampleSize);

// Check if the generated options match the expected result
expect(pageSizeOptions).toEqual(defaultPageOptions);
});

it('handles edge case when sample size is less than 0', () => {
const sampleSize = -10;

// Call the function
const pageSizeOptions = generatePageSizeOptions(sampleSize);

// Check if the generated options match the expected result
expect(pageSizeOptions).toEqual(defaultPageOptions);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright OpenSearch Contributors
* SPDX-License-Identifier: Apache-2.0
*/

import { uniq } from 'lodash';

/**
* Generates an array of pagination options based on the provided `sampleSize`.
* The array includes default values (25, 50, 100) and additional options derived from the `sampleSize` setting.
* Ensures uniqueness and sorts the array in ascending order, representing available page size options for pagination.
* @param {number} sampleSize - The sample size used to determine additional pagination options.
* @returns {number[]} - An array of available page size options.
*/

export const generatePageSizeOptions = (sampleSize: number): number[] => {
if (sampleSize && sampleSize > 0) {
const maxSize = 500;
Copy link
Member

Choose a reason for hiding this comment

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

Will stepSize sound more appropriate?

const pageSizeFromSetting = [...Array(Math.floor(sampleSize / maxSize)).keys()].map(
Copy link
Member

Choose a reason for hiding this comment

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

It seems the page size options is [25, 50, 100] when sampleSize is 400, is it expected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the idea was to provide a single page for a large dataset as per the issue. I think replacing 'floor' with 'ceil' works as expected. if you have any suggestions let me know.

(i) => (i + 1) * maxSize
);
return uniq([...defaultPageOptions, ...pageSizeFromSetting]).sort((a, b) => a - b);
Copy link
Member

Choose a reason for hiding this comment

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

We can replace uniq with Array.from(new Set()), we're getting rid of lodash as I know.

}
return defaultPageOptions;
};

export const defaultPageOptions = [25, 50, 100];
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,17 @@
import { renderHook, act } from '@testing-library/react-hooks';
import { usePagination } from './use_pagination';

import { uiSettingsServiceMock } from '../../../../../../core/public/mocks';

describe('usePagination', () => {
// TODO: create mock for DiscoverServices
const services: any = {
uiSettings: uiSettingsServiceMock.createSetupContract(),
};

it('should initialize correctly with visParams and nRow', () => {
const nRow = 30;
const { result } = renderHook(() => usePagination(nRow));
const { result } = renderHook(() => usePagination({ rowCount: nRow, services }));

expect(result.current).toEqual({
pageIndex: 0,
Expand All @@ -22,7 +29,7 @@ describe('usePagination', () => {

it('should update pageSize correctly when calling onChangeItemsPerPage', () => {
const nRow = 30;
const { result } = renderHook(() => usePagination(nRow));
const { result } = renderHook(() => usePagination({ rowCount: nRow, services }));

act(() => {
result.current?.onChangeItemsPerPage(20);
Expand All @@ -39,7 +46,7 @@ describe('usePagination', () => {

it('should update pageIndex correctly when calling onChangePage', () => {
const nRow = 30;
const { result } = renderHook(() => usePagination(nRow));
const { result } = renderHook(() => usePagination({ rowCount: nRow, services }));

act(() => {
result.current?.onChangePage(1);
Expand All @@ -56,7 +63,7 @@ describe('usePagination', () => {

it('should correct pageIndex if it exceeds maximum page index after nRow or perPage change', () => {
const nRow = 300;
const { result } = renderHook(() => usePagination(nRow));
const { result } = renderHook(() => usePagination({ rowCount: nRow, services }));

act(() => {
result.current?.onChangePage(4);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,25 @@
*/

import { useState, useMemo, useCallback } from 'react';
import { DiscoverServices } from '../../../build_services';
import { SAMPLE_SIZE_SETTING } from '../../../../common';
import { generatePageSizeOptions } from './page_size_options';
export interface Props {
services: DiscoverServices;
rowCount: number;
}

export const usePagination = ({ rowCount, services }: Props) => {
Copy link
Member

Choose a reason for hiding this comment

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

I'd avoid passing the entire services object, but passing pageSizeLimit here which is sampleSize in this case. Does it make sense?

Suggested change
export const usePagination = ({ rowCount, services }: Props) => {
export const usePagination = (rowCount: number, pageSizeLimit: number) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, thanks for the suggestions.

const { uiSettings } = services;

export const usePagination = (rowCount: number) => {
const [pagination, setPagination] = useState({ pageIndex: 0, pageSize: 100 });
const pageCount = useMemo(() => Math.ceil(rowCount / pagination.pageSize), [
rowCount,
pagination,
]);
const sampleSize = uiSettings.get(SAMPLE_SIZE_SETTING);

const pageSizeOptions = generatePageSizeOptions(sampleSize);

const onChangeItemsPerPage = useCallback(
(pageSize: number) => setPagination((p) => ({ ...p, pageSize })),
Expand All @@ -31,9 +43,9 @@ export const usePagination = (rowCount: number) => {
onChangePage,
pageIndex: pagination.pageIndex > pageCount - 1 ? 0 : pagination.pageIndex,
pageSize: pagination.pageSize,
pageSizeOptions: [25, 50, 100], // TODO: make this configurable
pageSizeOptions,
}
: undefined,
[pagination, onChangeItemsPerPage, onChangePage, pageCount]
[pagination, onChangeItemsPerPage, onChangePage, pageCount, pageSizeOptions]
);
};