From 7fdb5cece6d62867dc62d51a85ddb3551cfb84cc Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Wed, 13 Mar 2024 19:48:03 +0100 Subject: [PATCH 1/3] fix(list): improve select performance for big lists --- src/components/sectionList/SectionListRow.tsx | 21 ++-- .../sectionList/SectionListWrapper.tsx | 101 ++++++++---------- .../listActions/DefaultListActions.tsx | 4 +- .../sectionList/useSelectedModels.ts | 84 +++++++++++++++ 4 files changed, 144 insertions(+), 66 deletions(-) create mode 100644 src/components/sectionList/useSelectedModels.ts diff --git a/src/components/sectionList/SectionListRow.tsx b/src/components/sectionList/SectionListRow.tsx index 2febc669..4867dbd5 100644 --- a/src/components/sectionList/SectionListRow.tsx +++ b/src/components/sectionList/SectionListRow.tsx @@ -13,13 +13,18 @@ export type SectionListRowProps = { selectedColumns: SelectedColumns onSelect: (modelId: string, checked: boolean) => void selected: boolean - renderActions: (modelId: string) => React.ReactNode - renderColumnValue: (column: SelectedColumn) => React.ReactNode + renderActions: (model: Model) => React.ReactNode + renderColumnValue: ( + column: SelectedColumn, + modelData: Model + ) => React.ReactNode onClick?: (modelData: Model) => void active?: boolean } -export function SectionListRow({ +export const SectionListRow = React.memo(function SectionListRow< + Model extends BaseListModel +>({ active, selectedColumns, modelData, @@ -56,10 +61,14 @@ export function SectionListRow({ key={selectedColumn.path} onClick={() => onClick?.(modelData)} > - {renderColumnValue(selectedColumn)} + {renderColumnValue(selectedColumn, modelData)} ))} - {renderActions(modelData.id)} + {renderActions(modelData)} ) -} +}) + +export const MemoedSectionListRow = React.memo( + SectionListRow +) as typeof SectionListRow diff --git a/src/components/sectionList/SectionListWrapper.tsx b/src/components/sectionList/SectionListWrapper.tsx index a0b49124..8cb7639d 100644 --- a/src/components/sectionList/SectionListWrapper.tsx +++ b/src/components/sectionList/SectionListWrapper.tsx @@ -1,6 +1,6 @@ import { FetchError } from '@dhis2/app-runtime' import { SharingDialog } from '@dhis2/ui' -import React, { useMemo, useState } from 'react' +import React, { useCallback, useState } from 'react' import { BaseListModel, useSchemaFromHandle } from '../../lib' import { Pager, ModelCollection } from '../../types/models' import { SectionListHeaderBulk } from './bulk' @@ -17,6 +17,8 @@ import { SectionListEmpty, SectionListError } from './SectionListMessages' import { SectionListPagination } from './SectionListPagination' import { SectionListRow } from './SectionListRow' import { SectionListTitle } from './SectionListTitle' +import { SelectedColumn } from './types' +import { useSelectedModels } from './useSelectedModels' type SectionListWrapperProps = { data: ModelCollection | undefined @@ -31,40 +33,11 @@ export const SectionListWrapper = ({ }: SectionListWrapperProps) => { const { columns: headerColumns } = useModelListView() const schema = useSchemaFromHandle() - const [selectedModels, setSelectedModels] = useState>(new Set()) + const { selectedModels, isAllSelected, selectModel, selectAll, clearAll } = + useSelectedModels({ data }) const [detailsId, setDetailsId] = useState() const [sharingDialogId, setSharingDialogId] = useState() - const handleSelect = (id: string, checked: boolean) => { - if (checked) { - setSelectedModels(new Set(selectedModels).add(id)) - } else { - selectedModels.delete(id) - setSelectedModels(new Set(selectedModels)) - } - } - - const handleSelectAll = (checked: boolean) => { - if (checked) { - setSelectedModels( - new Set( - data?.map((model) => { - return model.id - }) - ) - ) - } else { - setSelectedModels(new Set()) - } - } - - const handleShowDetails = (id: string) => - setDetailsId((prevDetailsId) => (prevDetailsId === id ? undefined : id)) - - const allSelected = useMemo(() => { - return data?.length !== 0 && data?.length === selectedModels.size - }, [data, selectedModels.size]) - const SectionListMessage = () => { if (error) { console.log(error.details || error) @@ -79,6 +52,37 @@ export const SectionListWrapper = ({ return null } + const handleDetailsClick = useCallback( + ({ id }: BaseListModel) => { + setDetailsId((prevDetailsId) => + prevDetailsId === id ? undefined : id + ) + }, + [setDetailsId] + ) + + /* Note that SectionListRow is memoed, to prevent re-rendering + every item when interacting with a row */ + const renderColumnValue = useCallback( + ({ path }: SelectedColumn, model: BaseListModel) => { + return ( + + ) + }, + [schema] + ) + + const renderActions = useCallback( + (model: BaseListModel) => ( + + ), + [handleDetailsClick, setSharingDialogId] + ) + return (
@@ -87,15 +91,15 @@ export const SectionListWrapper = ({ {selectedModels.size > 0 ? ( setSelectedModels(new Set())} + onDeselectAll={clearAll} /> ) : ( )} {data?.map((model) => ( @@ -103,33 +107,14 @@ export const SectionListWrapper = ({ key={model.id} modelData={model} selectedColumns={headerColumns} - onSelect={handleSelect} - onClick={({ id }) => { - setDetailsId((prevDetailsId) => - prevDetailsId === id ? undefined : id - ) - }} + onSelect={selectModel} + onClick={handleDetailsClick} selected={selectedModels.has(model.id)} active={model.id === detailsId} - renderColumnValue={({ path }) => { - return ( - - ) - }} - renderActions={() => ( - - )} + renderColumnValue={renderColumnValue} + renderActions={renderActions} /> ))} - {detailsId && ( diff --git a/src/components/sectionList/listActions/DefaultListActions.tsx b/src/components/sectionList/listActions/DefaultListActions.tsx index 60d2715e..a78054f9 100644 --- a/src/components/sectionList/listActions/DefaultListActions.tsx +++ b/src/components/sectionList/listActions/DefaultListActions.tsx @@ -7,7 +7,7 @@ type ModelWithAccess = Pick type DefaultListActionProps = { model: ModelWithAccess - onShowDetailsClick: (id: string) => void + onShowDetailsClick: (model: ModelWithAccess) => void onOpenSharingClick: (id: string) => void } @@ -23,7 +23,7 @@ export const DefaultListActions = ({ onShowDetailsClick(model.id)} + onShowDetailsClick={() => onShowDetailsClick(model)} onOpenSharingClick={() => onOpenSharingClick(model.id)} /> diff --git a/src/components/sectionList/useSelectedModels.ts b/src/components/sectionList/useSelectedModels.ts new file mode 100644 index 00000000..f4b701ea --- /dev/null +++ b/src/components/sectionList/useSelectedModels.ts @@ -0,0 +1,84 @@ +// export interface useSelecto +import { useState, useMemo, useCallback } from 'react' +import { BaseListModel, canEditModel } from '../../lib' + +export function useSelectedModels({ + data, + initialSelected, +}: { + data: BaseListModel[] | undefined + initialSelected?: string[] +}) { + const [selectedModels, setSelectedModels] = useState>( + () => new Set(initialSelected || []) + ) + + const selectModel = useCallback( + (id: string, checked: boolean) => { + if (checked) { + setSelectedModels((prevSelected) => { + const newSelected = new Set(prevSelected) + newSelected.add(id) + return newSelected + }) + } else { + setSelectedModels((prevSelected) => { + const newSelected = new Set(prevSelected) + newSelected.delete(id) + return newSelected + }) + } + }, + [setSelectedModels] + ) + + const selectAll = useCallback( + (checked: boolean) => { + if (checked) { + setSelectedModels((prev) => { + const prevSeleted = Array.from(prev) + const allEditable = + data?.flatMap((model) => { + return canEditModel(model) ? [model.id] : [] + }) || [] + return new Set([...prevSeleted, ...allEditable]) + }) + } else { + setSelectedModels((prev) => { + // since data can be selected from multiple pages + // remove only selected models that are in the current data/page + // eg. keep selected models that are not in the current data + const newSelected = new Set(prev) + data?.forEach((m) => + prev.has(m.id) ? newSelected.delete(m.id) : null + ) + return new Set(newSelected) + }) + } + }, + [data, setSelectedModels] + ) + + const clearAll = useCallback( + () => setSelectedModels(new Set()), + [setSelectedModels] + ) + + const isAllSelected = useMemo(() => { + if (!data) { + return false + } + // do not include uneditable models when checking if all are selected + return data + .filter((m) => canEditModel(m)) + .every((model) => selectedModels.has(model.id)) + }, [data, selectedModels]) + + return { + selectedModels, + isAllSelected, + selectModel, + selectAll, + clearAll, + } +} From 0163eee0accb408e031dafdb5b2d30fba6e1b8e3 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Wed, 13 Mar 2024 20:11:16 +0100 Subject: [PATCH 2/3] fix: fix test --- src/lib/models/access.ts | 4 ++-- src/pages/dataElements/List.spec.tsx | 21 ++++++++++++++++----- src/types/generated/models.ts | 2 +- 3 files changed, 19 insertions(+), 8 deletions(-) diff --git a/src/lib/models/access.ts b/src/lib/models/access.ts index 85e122e0..b3d11d5b 100644 --- a/src/lib/models/access.ts +++ b/src/lib/models/access.ts @@ -1,9 +1,9 @@ import type { Access, BaseIdentifiableObject } from '../../types/generated' -export const editAccess = (access: Access) => access.write +export const hasWriteAccess = (access: Partial) => !!access.write export const canEditModel = (model: Pick) => - editAccess(model.access) + hasWriteAccess(model.access) export type ParsedAccessPart = { read: boolean diff --git a/src/pages/dataElements/List.spec.tsx b/src/pages/dataElements/List.spec.tsx index 126ef634..2d6f35ef 100644 --- a/src/pages/dataElements/List.spec.tsx +++ b/src/pages/dataElements/List.spec.tsx @@ -8,7 +8,7 @@ import { import userEvent from '@testing-library/user-event' import React from 'react' import dataElementSchemaMock from '../../__mocks__/schema/dataElementsSchema.json' -import { SECTIONS_MAP } from '../../lib' +import { SECTIONS_MAP, canEditModel } from '../../lib' import { useSchemaStore } from '../../lib/schemas/schemaStore' import { ModelSchemas } from '../../lib/useLoadApp' import TestComponentWithRouter, { @@ -291,13 +291,24 @@ describe('Data Elements List', () => { const allCheckBoxes = queryAllByTestId('section-list-row-checkbox') + // should only select checkboxes that are editable + const numberOfEditableItems = dataElementsMock.dataElements.reduce( + (acc, de) => (de && canEditModel(de) ? acc + 1 : acc), + 0 + ) expect(allCheckBoxes.length).toEqual(dataElementsMock.pager.pageSize) - // the UI library doesn't seem to apply checked on the input, which would have been a better test, so checking "checked" class on the svg icon + let numberOfSelectedBoxes = 0 allCheckBoxes.forEach((checkbox) => { - expect(checkbox.getElementsByTagName('svg')[0]).toHaveClass( - 'checked' - ) + // the UI library doesn't seem to apply checked on the input, which would have been a better test, so checking "checked" class on the svg icon + if ( + checkbox + .getElementsByTagName('svg')[0] + .classList.contains('checked') + ) { + numberOfSelectedBoxes++ + } }) + expect(numberOfSelectedBoxes).toEqual(numberOfEditableItems) }) // empty list diff --git a/src/types/generated/models.ts b/src/types/generated/models.ts index 10496b6d..ed712852 100644 --- a/src/types/generated/models.ts +++ b/src/types/generated/models.ts @@ -1,7 +1,7 @@ /* GENERATED BY https://github.com/Birkbjo/dhis2-open-api-ts */ export type Access = { - data: AccessData + data?: AccessData delete: boolean externalize: boolean manage: boolean From b7d6b8cd26c9a106e7517a66868c7da53d6c735d Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Wed, 13 Mar 2024 21:48:41 +0100 Subject: [PATCH 3/3] refactor: move data out of useSelectedModels --- .../sectionList/SectionListWrapper.tsx | 31 +++++- .../sectionList/useSelectedModels.ts | 102 ++++++++---------- 2 files changed, 72 insertions(+), 61 deletions(-) diff --git a/src/components/sectionList/SectionListWrapper.tsx b/src/components/sectionList/SectionListWrapper.tsx index 8cb7639d..56101986 100644 --- a/src/components/sectionList/SectionListWrapper.tsx +++ b/src/components/sectionList/SectionListWrapper.tsx @@ -1,7 +1,7 @@ import { FetchError } from '@dhis2/app-runtime' import { SharingDialog } from '@dhis2/ui' import React, { useCallback, useState } from 'react' -import { BaseListModel, useSchemaFromHandle } from '../../lib' +import { BaseListModel, canEditModel, useSchemaFromHandle } from '../../lib' import { Pager, ModelCollection } from '../../types/models' import { SectionListHeaderBulk } from './bulk' import { DetailsPanel, DefaultDetailsPanelContent } from './detailsPanel' @@ -33,8 +33,8 @@ export const SectionListWrapper = ({ }: SectionListWrapperProps) => { const { columns: headerColumns } = useModelListView() const schema = useSchemaFromHandle() - const { selectedModels, isAllSelected, selectModel, selectAll, clearAll } = - useSelectedModels({ data }) + const { selectedModels, checkAllSelected, add, remove, toggle, clearAll } = + useSelectedModels() const [detailsId, setDetailsId] = useState() const [sharingDialogId, setSharingDialogId] = useState() @@ -52,6 +52,25 @@ export const SectionListWrapper = ({ return null } + const handleSelectAll = useCallback( + (checked: boolean) => { + if (!data) { + return + } + if (checked) { + const editableIds = data + .filter((model) => canEditModel(model)) + .map((model) => model.id) + add(editableIds) + } else { + remove( + data.filter((model) => model.id).map((model) => model.id) + ) + } + }, + [data, add, remove] + ) + const handleDetailsClick = useCallback( ({ id }: BaseListModel) => { setDetailsId((prevDetailsId) => @@ -83,6 +102,8 @@ export const SectionListWrapper = ({ [handleDetailsClick, setSharingDialogId] ) + const isAllSelected = data ? checkAllSelected(data) : false + return (
@@ -98,7 +119,7 @@ export const SectionListWrapper = ({ )} @@ -107,7 +128,7 @@ export const SectionListWrapper = ({ key={model.id} modelData={model} selectedColumns={headerColumns} - onSelect={selectModel} + onSelect={toggle} onClick={handleDetailsClick} selected={selectedModels.has(model.id)} active={model.id === detailsId} diff --git a/src/components/sectionList/useSelectedModels.ts b/src/components/sectionList/useSelectedModels.ts index f4b701ea..8fbe8269 100644 --- a/src/components/sectionList/useSelectedModels.ts +++ b/src/components/sectionList/useSelectedModels.ts @@ -1,62 +1,48 @@ // export interface useSelecto +import { memoize } from 'lodash' import { useState, useMemo, useCallback } from 'react' import { BaseListModel, canEditModel } from '../../lib' -export function useSelectedModels({ - data, - initialSelected, -}: { - data: BaseListModel[] | undefined - initialSelected?: string[] -}) { +export type UseSelectedModelsOptions = { intialSelected?: string[] } + +export function useSelectedModels(options?: UseSelectedModelsOptions) { const [selectedModels, setSelectedModels] = useState>( - () => new Set(initialSelected || []) + () => new Set(options?.intialSelected || []) ) - const selectModel = useCallback( - (id: string, checked: boolean) => { - if (checked) { - setSelectedModels((prevSelected) => { - const newSelected = new Set(prevSelected) - newSelected.add(id) - return newSelected - }) - } else { - setSelectedModels((prevSelected) => { - const newSelected = new Set(prevSelected) - newSelected.delete(id) - return newSelected - }) - } + const add = useCallback( + (ids: string | string[]) => { + const addIds = Array.isArray(ids) ? ids : [ids] + setSelectedModels((prevSelected) => { + const newSelected = new Set(prevSelected) + addIds.forEach((id) => newSelected.add(id)) + return newSelected + }) }, [setSelectedModels] ) - const selectAll = useCallback( - (checked: boolean) => { + const remove = useCallback( + (ids: string | string[]) => { + const removeIds = Array.isArray(ids) ? ids : [ids] + + setSelectedModels((prevSelected) => { + const newSelected = new Set(prevSelected) + removeIds.forEach((id) => newSelected.delete(id)) + return newSelected + }) + }, + [setSelectedModels] + ) + const toggle = useCallback( + (ids: string | string[], checked: boolean) => { if (checked) { - setSelectedModels((prev) => { - const prevSeleted = Array.from(prev) - const allEditable = - data?.flatMap((model) => { - return canEditModel(model) ? [model.id] : [] - }) || [] - return new Set([...prevSeleted, ...allEditable]) - }) + add(ids) } else { - setSelectedModels((prev) => { - // since data can be selected from multiple pages - // remove only selected models that are in the current data/page - // eg. keep selected models that are not in the current data - const newSelected = new Set(prev) - data?.forEach((m) => - prev.has(m.id) ? newSelected.delete(m.id) : null - ) - return new Set(newSelected) - }) + remove(ids) } }, - [data, setSelectedModels] + [add, remove] ) const clearAll = useCallback( @@ -64,21 +50,25 @@ export function useSelectedModels({ [setSelectedModels] ) - const isAllSelected = useMemo(() => { - if (!data) { - return false - } - // do not include uneditable models when checking if all are selected - return data - .filter((m) => canEditModel(m)) - .every((model) => selectedModels.has(model.id)) - }, [data, selectedModels]) + const checkAllSelected = useMemo( + () => + memoize((data: BaseListModel[]) => { + if (!data) { + return false + } + return data + .filter((m) => canEditModel(m)) + .every((model) => selectedModels.has(model.id)) + }), + [selectedModels] + ) return { selectedModels, - isAllSelected, - selectModel, - selectAll, + checkAllSelected, + add, + remove, + toggle, clearAll, } }