From 791ecb2304b7d2eb766210c660889f59ae9b4115 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Mon, 11 Nov 2024 14:22:11 +0100 Subject: [PATCH 1/2] refactor(modelTransfer): simplify and fix refresh list crash (#429) [skip release] * refactor(modelTransfer): simplify and fix refresh list crash * reactor: minor cleanup --- .../ModelTransfer/BaseModelTransfer.tsx | 67 +++++++ .../ModelTransfer/ModelTransfer.tsx | 164 +++++++++--------- .../ModelTransfer/ModelTransferField.tsx | 76 ++------ src/types/models.ts | 5 + 4 files changed, 164 insertions(+), 148 deletions(-) create mode 100644 src/components/metadataFormControls/ModelTransfer/BaseModelTransfer.tsx diff --git a/src/components/metadataFormControls/ModelTransfer/BaseModelTransfer.tsx b/src/components/metadataFormControls/ModelTransfer/BaseModelTransfer.tsx new file mode 100644 index 00000000..22d37caf --- /dev/null +++ b/src/components/metadataFormControls/ModelTransfer/BaseModelTransfer.tsx @@ -0,0 +1,67 @@ +import { Transfer, TransferProps } from '@dhis2/ui' +import React, { useCallback, useMemo } from 'react' +import { DisplayableModel } from '../../../types/models' + +const toDisplayOption = (model: DisplayableModel) => ({ + value: model.id, + label: model.displayName, +}) + +type OwnProps = { + selected: TModel[] + available: TModel[] + onChange: ({ selected }: { selected: TModel[] }) => void +} + +export type BaseModelTransferProps = Omit< + TransferProps, + keyof OwnProps | 'options' +> & + OwnProps + +/* Simple wrapper component handle generic models with Transfer-component. */ +export const BaseModelTransfer = ({ + available, + selected, + onChange, + ...transferProps +}: BaseModelTransferProps) => { + const { allModelsMap, allTransferOptions } = useMemo(() => { + const allModels = selected.concat(available) + const allModelsMap = new Map(allModels.map((o) => [o.id, o])) + const allTransferOptions = allModels.map(toDisplayOption) + return { + allModelsMap, + allTransferOptions, + } + }, [available, selected]) + + const selectedTransferValues = useMemo( + () => selected.map((s) => s.id), + [selected] + ) + + const handleOnChange = useCallback( + ({ selected }: { selected: string[] }) => { + // map the selected ids to the full model + // loop through selected to keep order + const selectedModels = selected + .map((id) => allModelsMap.get(id)) + .filter((model) => !!model) + + onChange({ + selected: selectedModels, + }) + }, + [onChange, allModelsMap] + ) + + return ( + + ) +} diff --git a/src/components/metadataFormControls/ModelTransfer/ModelTransfer.tsx b/src/components/metadataFormControls/ModelTransfer/ModelTransfer.tsx index 1f28e5da..ce0ec5a9 100644 --- a/src/components/metadataFormControls/ModelTransfer/ModelTransfer.tsx +++ b/src/components/metadataFormControls/ModelTransfer/ModelTransfer.tsx @@ -1,23 +1,20 @@ -import { Transfer, TransferProps } from '@dhis2/ui' -import React, { - forwardRef, - RefAttributes, - useImperativeHandle, - useMemo, - useState, -} from 'react' +import i18n from '@dhis2/d2-i18n' +import { Button, ButtonStrip } from '@dhis2/ui' +import React, { useMemo, useState } from 'react' import { useInfiniteQuery } from 'react-query' +import { useHref } from 'react-router-dom' +import { useDebouncedCallback } from 'use-debounce' +import { getSectionNewPath } from '../../../lib' import { useBoundResourceQueryFn } from '../../../lib/query/useBoundQueryFn' import { PlainResourceQuery } from '../../../types' import { PagedResponse } from '../../../types/generated' +import { DisplayableModel } from '../../../types/models' +import { LinkButton } from '../../LinkButton' +import { BaseModelTransfer, BaseModelTransferProps } from './BaseModelTransfer' +import css from './ModelTransfer.module.css' type Response = PagedResponse -export type DisplayableModel = { - id: string - displayName: string -} - const defaultQuery = { params: { order: 'displayName:asc', @@ -25,32 +22,23 @@ const defaultQuery = { }, } -const toDisplayOption = (model: DisplayableModel) => ({ - value: model.id, - label: model.displayName, -}) - -type OwnProps = { - selected: TModel[] - onChange: ({ selected }: { selected: TModel[] }) => void - query: PlainResourceQuery +export type ModelTranferProps = Omit< + BaseModelTransferProps, + 'available' | 'filterable' +> & { + query: Omit } -type ModelTransferProps = Omit< - TransferProps, - | keyof OwnProps - | 'options' - | 'selected' - | 'filterable' - | 'onFilterChange' -> & - OwnProps -type ImperativeRef = { refetch: () => void } - -const BaseModelTransfer = ( - { selected, onChange, query, ...transferProps }: ModelTransferProps, - ref: React.Ref -) => { +export const ModelTransfer = ({ + selected, + query, + leftHeader, + rightHeader, + leftFooter, + filterPlaceholder, + filterPlaceholderPicked, + ...baseModelTransferProps +}: ModelTranferProps) => { const queryFn = useBoundResourceQueryFn() const [searchTerm, setSearchTerm] = useState('') @@ -67,6 +55,7 @@ const BaseModelTransfer = ( }, } const modelName = query.resource + const newLink = useHref(`/${getSectionNewPath(modelName)}`) const queryResult = useInfiniteQuery({ queryKey: [queryObject] as const, @@ -77,64 +66,73 @@ const BaseModelTransfer = ( getPreviousPageParam: (firstPage) => firstPage.pager.prevPage ? firstPage.pager.page - 1 : undefined, }) - - useImperativeHandle(ref, () => ({ - refetch: () => queryResult.refetch(), - })) - const allDataMap = useMemo( - () => - new Map( - queryResult.data?.pages.flatMap((page) => - page[modelName].map((d) => [d.id, d] as const) - ) - ), + () => queryResult.data?.pages.flatMap((page) => page[modelName]) ?? [], [queryResult.data, modelName] ) - const selectedOptions = selected ? selected?.map(toDisplayOption) : [] - const loadedOptions = Array.from(allDataMap.values()).map(toDisplayOption) - // always include selected options - const allOptions = selectedOptions.concat(loadedOptions || []) - - const handleOnChange = ({ selected }: { selected: string[] }) => { - // map the selected ids to the full model - // loop through selected to keep order - const selectedModels = selected - .map((id) => allDataMap.get(id)) - .filter((model): model is TModel => !!model) - - onChange({ - selected: selectedModels, - }) - } + const handleFilterChange = useDebouncedCallback(({ value }) => { + if (value != undefined) { + setSearchTerm(value) + } + }, 250) return ( - - value !== undefined && setSearchTerm(value) + available={allDataMap} + selected={selected} + onFilterChange={handleFilterChange} + filterPlaceholder={ + filterPlaceholder || i18n.t('Filter available items') + } + filterPlaceholderPicked={ + filterPlaceholderPicked || i18n.t('Filter selected items') + } + leftHeader={{leftHeader}} + rightHeader={{rightHeader}} + leftFooter={ + leftFooter ?? ( + + ) } - selected={selectedOptions.map((o) => o.value)} - onChange={handleOnChange} + {...baseModelTransferProps} /> ) } -// this is needed to support generics with ref-forwarding -interface ModelTransferWithForwardedRef - extends React.FC> { - ( - props: ModelTransferProps & RefAttributes - ): React.ReactNode +const TransferHeader = ({ children }: { children: React.ReactNode }) => { + if (typeof children === 'string') { + return
{children}
+ } + return <>{children} } -export const ModelTransfer: ModelTransferWithForwardedRef = - forwardRef(BaseModelTransfer) +const DefaultTransferLeftFooter = ({ + onRefreshClick, + newLink, +}: { + onRefreshClick: () => void + newLink: string +}) => { + return ( + + + + + {i18n.t('Add new')} + + + ) +} diff --git a/src/components/metadataFormControls/ModelTransfer/ModelTransferField.tsx b/src/components/metadataFormControls/ModelTransfer/ModelTransferField.tsx index 2a04a0a7..cd9e4640 100644 --- a/src/components/metadataFormControls/ModelTransfer/ModelTransferField.tsx +++ b/src/components/metadataFormControls/ModelTransfer/ModelTransferField.tsx @@ -1,12 +1,9 @@ -import i18n from '@dhis2/d2-i18n' -import { Button, ButtonStrip, Field, TransferProps } from '@dhis2/ui' -import React, { useRef } from 'react' +import { Field, TransferProps } from '@dhis2/ui' +import React from 'react' import { useField } from 'react-final-form' -import { useHref } from 'react-router' -import { DisplayableModel, ModelTransfer } from '../../../components' -import { getSectionNewPath } from '../../../lib' +import { ModelTransfer } from '../../../components' import { PlainResourceQuery } from '../../../types' -import { LinkButton } from '../../LinkButton' +import { DisplayableModel } from '../../../types/models' import css from './ModelTransfer.module.css' // this currently does not need a generic, because the value of the field is not passed @@ -30,25 +27,17 @@ export function ModelTransferField({ name, query, label, - rightHeader, leftHeader, - rightFooter, + rightHeader, leftFooter, + rightFooter, filterPlaceholder, filterPlaceholderPicked, }: ModelTransferFieldProps) { - const modelName = query.resource const { input, meta } = useField(name, { multiple: true, validateFields: [], }) - const newLink = useHref(`/${getSectionNewPath(modelName)}`) - - const modelTransferHandle = useRef({ - refetch: () => { - throw new Error('Not initialized') - }, - }) return ( { input.onChange(selected) input.onBlur() }} - leftHeader={{leftHeader}} - rightHeader={{rightHeader}} - leftFooter={ - leftFooter ?? ( - - ) - } + leftHeader={leftHeader} + rightHeader={rightHeader} + leftFooter={leftFooter} rightFooter={rightFooter} + filterPlaceholder={filterPlaceholder} + filterPlaceholderPicked={filterPlaceholderPicked} query={query} - height={'350px'} - optionsWidth="500px" - selectedWidth="500px" - enableOrderChange={true} /> ) } - -const TransferHeader = ({ children }: { children: React.ReactNode }) => { - if (typeof children === 'string') { - return
{children}
- } - return <>{children} -} - -const DefaultTransferLeftFooter = ({ - onRefreshClick, - newLink, -}: { - onRefreshClick: () => void - newLink: string -}) => { - return ( - - - - - {i18n.t('Add new')} - - - ) -} diff --git a/src/types/models.ts b/src/types/models.ts index 0b0f35a1..87702648 100644 --- a/src/types/models.ts +++ b/src/types/models.ts @@ -1,3 +1,8 @@ // generated by https://github.com/Birkbjo/dhis2-open-api-ts export type * from './generated' export * from './systemSettings' + +export type DisplayableModel = { + id: string + displayName: string +} From e2dc678cf000c21dea9f4e46779681bfb791e600 Mon Sep 17 00:00:00 2001 From: Birk Johansson Date: Mon, 11 Nov 2024 14:22:54 +0100 Subject: [PATCH 2/2] refactor(breadcrumbs): simplify and refactor breadcrumbs (#434) * refactor: simplify and improve breadcrumbitem * fix: fix tests * fix: improve route handle type --- i18n/en.pot | 31 +++++++++++- src/app/layout/Breadcrumb.module.css | 18 ++++--- src/app/layout/Breadcrumb.spec.tsx | 45 ++++++++++-------- src/app/layout/Breadcrumb.tsx | 45 +++++++++++------- src/app/routes/Router.tsx | 71 ++++++++++++++++++++++------ src/app/routes/types.ts | 10 ++-- 6 files changed, 154 insertions(+), 66 deletions(-) diff --git a/i18n/en.pot b/i18n/en.pot index 17fadb7c..baf333f8 100644 --- a/i18n/en.pot +++ b/i18n/en.pot @@ -5,8 +5,8 @@ msgstr "" "Content-Type: text/plain; charset=utf-8\n" "Content-Transfer-Encoding: 8bit\n" "Plural-Forms: nplurals=2; plural=(n != 1)\n" -"POT-Creation-Date: 2024-11-05T07:23:15.731Z\n" -"PO-Revision-Date: 2024-11-05T07:23:15.732Z\n" +"POT-Creation-Date: 2024-11-05T14:43:19.245Z\n" +"PO-Revision-Date: 2024-11-05T14:43:19.245Z\n" msgid "schemas" msgstr "schemas" @@ -33,6 +33,12 @@ msgstr "Not found" msgid "The page you are looking for does not exist." msgstr "The page you are looking for does not exist." +msgid "New {{modelName}}" +msgstr "New {{modelName}}" + +msgid "Edit {{modelName}}" +msgstr "Edit {{modelName}}" + msgid "Metadata Overview" msgstr "Metadata Overview" @@ -1154,6 +1160,27 @@ msgstr "" "included. PHU will still be available for the PHU level, but not included " "in the aggregations to the levels above." +msgid "Setup" +msgstr "Setup" + +msgid "Data" +msgstr "Data" + +msgid "Data Elements" +msgstr "Data Elements" + +msgid "Periods" +msgstr "Periods" + +msgid "Organisation Units" +msgstr "Organisation Units" + +msgid "Form" +msgstr "Form" + +msgid "Advanced" +msgstr "Advanced" + msgid "Longitude" msgstr "Longitude" diff --git a/src/app/layout/Breadcrumb.module.css b/src/app/layout/Breadcrumb.module.css index 0dc3757f..7a812c2a 100644 --- a/src/app/layout/Breadcrumb.module.css +++ b/src/app/layout/Breadcrumb.module.css @@ -2,28 +2,32 @@ margin-bottom: 8px; } -.breadcrumbItem { - color: var(--colors-blue600); - text-decoration: none; +.breadCrumbItem { font-size: 14px; line-height: 18px; } -span.breadcrumbItem { +.breadcrumbItemLink { + composes: breadCrumbItem; + color: var(--colors-blue600); + text-decoration: none; +} + +span.breadcrumbItemLink { cursor: pointer; } -.breadcrumbItem:hover { +.breadcrumbItemLink:hover { text-decoration: underline; color: var(--colors-blue700); } -.breadcrumbItem:active { +.breadcrumbItemLink:active { text-decoration: underline; color: var(--colors-blue900); } -.breadcrumbItem:focus { +.breadcrumbItemLink:focus { outline: 1px solid var(--colors-blue600); } diff --git a/src/app/layout/Breadcrumb.spec.tsx b/src/app/layout/Breadcrumb.spec.tsx index 03ce6aa6..98eedc42 100644 --- a/src/app/layout/Breadcrumb.spec.tsx +++ b/src/app/layout/Breadcrumb.spec.tsx @@ -2,7 +2,12 @@ import '@testing-library/jest-dom' import { configure, render } from '@testing-library/react' import React from 'react' import { useMatches, HashRouter } from 'react-router-dom' -import { OVERVIEW_SECTIONS, SECTIONS_MAP } from '../../lib' +import { + getOverviewPath, + getSectionPath, + OVERVIEW_SECTIONS, + SECTIONS_MAP, +} from '../../lib' import { MatchRouteHandle, RouteHandle } from '../routes/types' import { Breadcrumbs, BreadcrumbItem } from './Breadcrumb' @@ -34,9 +39,12 @@ beforeEach(() => { }) describe('BreadcrumbItem', () => { - it('should render a link based on the section title', () => { + it('should render a link based on the given label', () => { const { getByRole } = render( - , + , { wrapper: HashRouter } ) @@ -46,9 +54,12 @@ describe('BreadcrumbItem', () => { expect(DataElementLink).toHaveAttribute('href', '#/dataElements') }) - it('should render a link to overview-section using plural title if overview-section', () => { + it('should render a correct link to overview-section using plural title if overview-section', () => { const { getByRole } = render( - , + , { wrapper: HashRouter } ) @@ -60,20 +71,6 @@ describe('BreadcrumbItem', () => { '#/overview/dataElements' ) }) - it('should render a link with label-prop instead of section title if provided', () => { - const { getByRole } = render( - , - { wrapper: HashRouter } - ) - - const DataElementLink = getByRole('link') - expect(DataElementLink).toBeDefined() - expect(DataElementLink).toHaveTextContent('Data elements') - expect(DataElementLink).toHaveAttribute('href', '#/dataElements') - }) }) describe('Breadcrumbs', () => { it('should render crumb components in handle', () => { @@ -97,13 +94,19 @@ describe('Breadcrumbs', () => { mockHandle({ crumb: () => ( ), }), mockHandle({ crumb: () => ( - + ), }), ]) diff --git a/src/app/layout/Breadcrumb.tsx b/src/app/layout/Breadcrumb.tsx index 890b05fa..9d128b89 100644 --- a/src/app/layout/Breadcrumb.tsx +++ b/src/app/layout/Breadcrumb.tsx @@ -1,36 +1,44 @@ import React from 'react' -import { Link, useMatches } from 'react-router-dom' -import { - Section, - isOverviewSection, - getSectionPath, - getOverviewPath, - useToWithSearchState, -} from '../../lib' +import { Link, To, useLocation, useMatches, matchPath } from 'react-router-dom' +import { useToWithSearchState } from '../../lib' import type { MatchRouteHandle } from '../routes/types' import css from './Breadcrumb.module.css' const BreadcrumbSeparator = () => / type BreadcrumbItemProps = { - section: Section - label?: string + label: string + to: To } -export const BreadcrumbItem = ({ section, label }: BreadcrumbItemProps) => { - const isOverview = isOverviewSection(section) - const link = isOverview ? getOverviewPath(section) : getSectionPath(section) - const to = useToWithSearchState(`/${link}`) +export const BreadcrumbItem = ({ label, to }: BreadcrumbItemProps) => { + const resolvedTo = useToWithSearchState(to) + const currentLoc = useLocation() - label = label ?? isOverview ? section.titlePlural : section.title + if (resolvedTo.pathname) { + const match = matchPath(resolvedTo.pathname, currentLoc.pathname) + if (match?.pattern.end) { + return + } + } return ( - + {label} ) } +/** Component that is used for "End links", where the current route is the end of the path + * and thus should not be a link */ +export const BreadCrumbEndItem = ({ label }: { label: string }) => ( + {label} +) + export const Breadcrumbs = () => { const matches = useMatches() as MatchRouteHandle[] @@ -38,7 +46,10 @@ export const Breadcrumbs = () => { .filter((match) => match.handle?.crumb) .map((match) => ( - {match.handle?.crumb?.()} + {match.handle?.crumb?.({ + params: match.params, + pathname: match.pathname, + })} )) diff --git a/src/app/routes/Router.tsx b/src/app/routes/Router.tsx index df1ddd24..02e4b343 100644 --- a/src/app/routes/Router.tsx +++ b/src/app/routes/Router.tsx @@ -1,3 +1,4 @@ +import i18n from '@dhis2/d2-i18n' import React from 'react' import { createHashRouter, @@ -20,13 +21,14 @@ import { isModuleNotFoundError, isValidUid, routePaths, + getOverviewPath, } from '../../lib' import { OverviewSection } from '../../types' import { Layout, Breadcrumbs, BreadcrumbItem } from '../layout' import { CheckAuthorityForSection } from './CheckAuthorityForSection' import { DefaultErrorRoute } from './DefaultErrorRoute' import { LegacyAppRedirect } from './LegacyAppRedirect' - +import { RouteHandle } from './types' // This loads all the overview routes in the same chunk since they resolve to the same promise // see https://reactrouter.com/en/main/route/lazy#multiple-routes-in-a-single-file // Overviews are small, and the AllOverview would load all the other overviews anyway, @@ -109,14 +111,22 @@ const schemaSectionRoutes = Object.values(SCHEMA_SECTIONS).map((section) => ( ( - - ), - }} + handle={ + { + section, + crumb: () => ( + + ), + } satisfies RouteHandle + } element={ <> @@ -126,21 +136,52 @@ const schemaSectionRoutes = Object.values(SCHEMA_SECTIONS).map((section) => ( > , - }} + handle={ + { + hideSidebar: true, + crumb: (matchInfo) => ( + + ), + } satisfies RouteHandle + } > {!sectionsNoNewRoute.has(section) && ( ( + + ), + } satisfies RouteHandle + } /> )} }> ( + + ), + } satisfies RouteHandle + } lazy={createSectionLazyRouteFunction(section, 'Edit')} /> @@ -175,7 +216,7 @@ const routes = createRoutesFromElements( section.componentName, section )} - handle={{ section }} + handle={{ section } satisfies RouteHandle} /> ))} diff --git a/src/app/routes/types.ts b/src/app/routes/types.ts index e280b69f..62429dd6 100644 --- a/src/app/routes/types.ts +++ b/src/app/routes/types.ts @@ -1,17 +1,19 @@ -import type { useMatches } from 'react-router-dom' -import type { ModelSection } from '../../types' +import type { UIMatch, useMatches } from 'react-router-dom' +import type { ModelSection, OverviewSection } from '../../types' // utility type to type a match with a handle-property returned from useMatches // since handle is unknown, we need to cast it to the correct type type MatchWithHandle = ReturnType[number] & { handle?: THandle } +export type BreadCrumbMatchInfo = Pick + // common type for possible handle-properties used in Route export type RouteHandle = { hideSidebar?: boolean - section?: ModelSection + section?: ModelSection | OverviewSection showFooter?: boolean - crumb?: () => React.ReactNode + crumb?: (matchInfo: BreadCrumbMatchInfo) => React.ReactNode } export type MatchRouteHandle = MatchWithHandle