From 8ba66bd0ac044759e1ca70ba4566cfbe1af9897b Mon Sep 17 00:00:00 2001 From: Gustaf Lundh Date: Sun, 20 Nov 2022 22:53:31 +0100 Subject: [PATCH] Fixed review comments * Now we don't react on external changes to the query kindParameter * Casing is always correct (APIs instead of Apis etc) Signed-off-by: Gustaf Lundh --- .changeset/brave-bags-sniff.md | 1 + .changeset/neat-lies-know.md | 2 +- plugins/catalog-react/api-report.md | 6 ++ .../EntityKindPicker.test.tsx | 62 ++++--------------- .../EntityKindPicker/EntityKindPicker.tsx | 12 +--- plugins/catalog-react/src/index.ts | 2 +- plugins/catalog-react/src/utils/index.ts | 2 +- .../src/utils/kindFilterUtils.ts | 43 +++++++------ .../CatalogKindHeader.test.tsx | 38 +----------- .../CatalogKindHeader/CatalogKindHeader.tsx | 12 +--- 10 files changed, 51 insertions(+), 129 deletions(-) diff --git a/.changeset/brave-bags-sniff.md b/.changeset/brave-bags-sniff.md index 2608ca7f1e..3fa542a4e3 100644 --- a/.changeset/brave-bags-sniff.md +++ b/.changeset/brave-bags-sniff.md @@ -3,3 +3,4 @@ --- Fixes in kind selectors (now OwnershipCards work again) +EntityKindPicker now accepts an optional allowedKinds prop, just like CatalogKindHeader. diff --git a/.changeset/neat-lies-know.md b/.changeset/neat-lies-know.md index cb355c638f..01fc002668 100644 --- a/.changeset/neat-lies-know.md +++ b/.changeset/neat-lies-know.md @@ -1,5 +1,5 @@ --- -'@backstage/plugin-catalog-react': minor +'@backstage/plugin-catalog-react': patch --- Cleanup and small fixes for the kind selector diff --git a/plugins/catalog-react/api-report.md b/plugins/catalog-react/api-report.md index ea67702c9b..91c57c3498 100644 --- a/plugins/catalog-react/api-report.md +++ b/plugins/catalog-react/api-report.md @@ -432,6 +432,9 @@ export type FavoriteEntityProps = ComponentProps & { entity: Entity; }; +// Warning: (tsdoc-undefined-tag) The TSDoc tag "@private" is not defined in this configuration +// Warning: (ae-missing-release-tag) "filterAndCapitalize" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// // @public export function filterAndCapitalize( allKinds: string[], @@ -511,6 +514,9 @@ export type UnregisterEntityDialogProps = { entity: Entity; }; +// Warning: (tsdoc-undefined-tag) The TSDoc tag "@private" is not defined in this configuration +// Warning: (ae-missing-release-tag) "useAllKinds" is exported by the package, but it is missing a release tag (@alpha, @beta, @public, or @internal) +// // @public export function useAllKinds(): { loading: boolean; diff --git a/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.test.tsx b/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.test.tsx index 784580b636..04d8df3d16 100644 --- a/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.test.tsx +++ b/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.test.tsx @@ -148,21 +148,21 @@ describe('', () => { }); it('renders unknown kinds provided in query parameters', async () => { - const rendered = await renderWithEffects( + await renderWithEffects( , ); - expect(rendered.getByText('Frob')).toBeInTheDocument(); + expect(screen.getByText('FROb')).toBeInTheDocument(); }); it('limits kinds when allowedKinds is set', async () => { - const rendered = await renderWithEffects( + await renderWithEffects( @@ -170,56 +170,20 @@ describe('', () => { , ); - const input = rendered.getByTestId('select'); + const input = screen.getByTestId('select'); fireEvent.click(input); expect( - rendered.getByRole('option', { name: 'Component' }), + screen.getByRole('option', { name: 'Component' }), ).toBeInTheDocument(); + expect(screen.getByRole('option', { name: 'Domain' })).toBeInTheDocument(); expect( - rendered.getByRole('option', { name: 'Domain' }), - ).toBeInTheDocument(); - expect( - rendered.queryByRole('option', { name: 'Template' }), + screen.queryByRole('option', { name: 'Template' }), ).not.toBeInTheDocument(); }); - it('responds to external queryParameters changes', async () => { - const updateFilters = jest.fn(); - const rendered = await renderWithEffects( - - - - - , - ); - expect(updateFilters).toHaveBeenLastCalledWith({ - kind: new EntityKindFilter('component'), - }); - rendered.rerender( - - - - - , - ); - expect(updateFilters).toHaveBeenLastCalledWith({ - kind: new EntityKindFilter('domain'), - }); - }); - it('renders kind from the query parameter even when not in allowedKinds', async () => { - const rendered = await renderWithEffects( + await renderWithEffects( ', () => { , ); - expect(rendered.getByText('Frob')).toBeInTheDocument(); + expect(screen.getByText('Frob')).toBeInTheDocument(); - const input = rendered.getByTestId('select'); + const input = screen.getByTestId('select'); fireEvent.click(input); - expect( - rendered.getByRole('option', { name: 'Domain' }), - ).toBeInTheDocument(); + expect(screen.getByRole('option', { name: 'Domain' })).toBeInTheDocument(); }); }); diff --git a/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.tsx b/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.tsx index 89313790f5..b0d1565cfb 100644 --- a/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.tsx +++ b/plugins/catalog-react/src/components/EntityKindPicker/EntityKindPicker.tsx @@ -20,7 +20,7 @@ import { Box } from '@material-ui/core'; import React, { useEffect, useMemo, useState } from 'react'; import { EntityKindFilter } from '../../filters'; import { useEntityList } from '../../hooks'; -import { filterAndCapitalize, useAllKinds } from '../../utils/kindFilterUtils'; +import { filterKinds, useAllKinds } from '../../utils/kindFilterUtils'; function useEntityKindFilter(opts: { initialFilter: string }): { loading: boolean; @@ -58,14 +58,6 @@ function useEntityKindFilter(opts: { initialFilter: string }): { }); }, [selectedKind, updateFilters]); - // Set selected kinds on query parameter updates; this happens at initial page load and from - // external updates to the page location. - useEffect(() => { - if (queryParamKind) { - setSelectedKind(queryParamKind); - } - }, [queryParamKind]); - const { allKinds, loading, error } = useAllKinds(); return { @@ -114,7 +106,7 @@ export const EntityKindPicker = (props: EntityKindPickerProps) => { if (error) return null; - const options = filterAndCapitalize(allKinds, allowedKinds, [selectedKind]); + const options = filterKinds(allKinds, allowedKinds, selectedKind); const items = Object.keys(options).map(key => ({ value: key, diff --git a/plugins/catalog-react/src/index.ts b/plugins/catalog-react/src/index.ts index 783b9c1959..de8b32282a 100644 --- a/plugins/catalog-react/src/index.ts +++ b/plugins/catalog-react/src/index.ts @@ -36,6 +36,6 @@ export { getEntitySourceLocation, isOwnerOf, useAllKinds, - filterAndCapitalize, + filterKinds, } from './utils'; export type { EntitySourceLocation } from './utils'; diff --git a/plugins/catalog-react/src/utils/index.ts b/plugins/catalog-react/src/utils/index.ts index 197eff3a55..8262d1cc81 100644 --- a/plugins/catalog-react/src/utils/index.ts +++ b/plugins/catalog-react/src/utils/index.ts @@ -18,4 +18,4 @@ export { getEntityRelations } from './getEntityRelations'; export { getEntitySourceLocation } from './getEntitySourceLocation'; export type { EntitySourceLocation } from './getEntitySourceLocation'; export { isOwnerOf } from './isOwnerOf'; -export { useAllKinds, filterAndCapitalize } from './kindFilterUtils'; +export { useAllKinds, filterKinds } from './kindFilterUtils'; diff --git a/plugins/catalog-react/src/utils/kindFilterUtils.ts b/plugins/catalog-react/src/utils/kindFilterUtils.ts index 939c7296dd..a0441db33f 100644 --- a/plugins/catalog-react/src/utils/kindFilterUtils.ts +++ b/plugins/catalog-react/src/utils/kindFilterUtils.ts @@ -15,14 +15,12 @@ */ import { useApi } from '@backstage/core-plugin-api'; -import { capitalize } from '@material-ui/core'; import useAsync from 'react-use/lib/useAsync'; import { catalogApiRef } from '../api'; /** * Fetch and return all availible kinds. - * - * @public + * @internal */ export function useAllKinds(): { loading: boolean; @@ -48,35 +46,40 @@ export function useAllKinds(): { /** * Filter and capitalize accessible kinds. * - * @public + * @internal */ -export function filterAndCapitalize( +export function filterKinds( allKinds: string[], allowedKinds?: string[], - forcedKinds?: string[], + forcedKinds?: string, ): Record { // Before allKinds is loaded, or when a kind is entered manually in the URL, selectedKind may not // be present in allKinds. It should still be shown in the dropdown, but may not have the nice // enforced casing from the catalog-backend. This makes a key/value record for the Select options, // including selectedKind if it's unknown - but allows the selectedKind to get clobbered by the // more proper catalog kind if it exists. - const availableKinds = allKinds - .concat(forcedKinds ?? []) - .filter(k => - allowedKinds - ? allowedKinds.some( - a => a.toLocaleLowerCase('en-US') === k.toLocaleLowerCase('en-US'), - ) || - forcedKinds?.some( - f => f.toLocaleLowerCase('en-US') === k.toLocaleLowerCase('en-US'), - ) - : true, + let availableKinds = allKinds; + if (allowedKinds) { + availableKinds = availableKinds.filter(k => + allowedKinds.some( + a => a.toLocaleLowerCase('en-US') === k.toLocaleLowerCase('en-US'), + ), ); + } + if ( + forcedKinds && + !allKinds.some( + a => + a.toLocaleLowerCase('en-US') === forcedKinds.toLocaleLowerCase('en-US'), + ) + ) { + availableKinds = availableKinds.concat([forcedKinds]); + } - const capitalizedKinds = availableKinds.sort().reduce((acc, kind) => { - acc[kind.toLocaleLowerCase('en-US')] = capitalize(kind); + const kindsMap = availableKinds.sort().reduce((acc, kind) => { + acc[kind.toLocaleLowerCase('en-US')] = kind; return acc; }, {} as Record); - return capitalizedKinds; + return kindsMap; } diff --git a/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.test.tsx b/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.test.tsx index 9f68252ca7..acff9e3cda 100644 --- a/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.test.tsx +++ b/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.test.tsx @@ -105,14 +105,14 @@ describe('', () => { await renderWithEffects( , ); - expect(screen.getByText('Frobs')).toBeInTheDocument(); + expect(screen.getByText('FRObs')).toBeInTheDocument(); }); it('updates the kind filter', async () => { @@ -136,40 +136,6 @@ describe('', () => { }); }); - it('responds to external queryParameters changes', async () => { - const updateFilters = jest.fn(); - const rendered = await renderWithEffects( - - - - - , - ); - expect(updateFilters).toHaveBeenLastCalledWith({ - kind: new EntityKindFilter('components'), - }); - rendered.rerender( - - - - - , - ); - expect(updateFilters).toHaveBeenLastCalledWith({ - kind: new EntityKindFilter('template'), - }); - }); - it('limits kinds when allowedKinds is set', async () => { await renderWithEffects( diff --git a/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.tsx b/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.tsx index 7de842abe6..5a69f89106 100644 --- a/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.tsx +++ b/plugins/catalog/src/components/CatalogKindHeader/CatalogKindHeader.tsx @@ -25,7 +25,7 @@ import { } from '@material-ui/core'; import { EntityKindFilter, - filterAndCapitalize, + filterKinds, useAllKinds, useEntityList, } from '@backstage/plugin-catalog-react'; @@ -90,15 +90,7 @@ export function CatalogKindHeader(props: CatalogKindHeaderProps) { }); }, [selectedKind, updateFilters]); - // Set selected Kind on query parameter updates; this happens at initial page load and from - // external updates to the page location. - useEffect(() => { - if (queryParamKind) { - setSelectedKind(queryParamKind); - } - }, [queryParamKind]); - - const options = filterAndCapitalize(allKinds, allowedKinds, [selectedKind]); + const options = filterKinds(allKinds, allowedKinds, selectedKind); return (