Merge pull request #1199 from spotify/freben/unregister

Unbreak the unregister dialog after catalog changes

packages/app/src/components/Root/Root.tsx

@@ -57,11 +57,14 @@ const SidebarLogo: FC<{}> = () => {
 
   return (
     <div className={classes.root}>
-      <NavLink to="/">
-        <Link underline="none" className={classes.link}>
-          {isOpen ? <LogoFull /> : <LogoIcon />}
-        </Link>
-      </NavLink>
+      <Link
+        component={NavLink}
+        to="/"
+        underline="none"
+        className={classes.link}
+      >
+        {isOpen ? <LogoFull /> : <LogoIcon />}
+      </Link>
     </div>
   );
 };

plugins/catalog/src/api/types.ts

@@ -24,7 +24,6 @@ export const catalogApiRef = createApiRef<CatalogApi>({
 
 export interface CatalogApi {
   getLocationById(id: String): Promise<Location | undefined>;
-  removeLocationById(id: String): Promise<void>;
   getEntities(filter?: Record<string, string>): Promise<Entity[]>;
   getEntityByName(name: string): Promise<Entity>;
   addLocation(type: string, target: string): Promise<AddLocationResponse>;

plugins/catalog/src/components/CatalogPage/CatalogPage.tsx

@@ -35,7 +35,7 @@ import { useAsync } from 'react-use';
 import { catalogApiRef } from '../..';
 import { Component } from '../../data/component';
 import { defaultFilter, filterGroups } from '../../data/filters';
-import { entityToComponent, findLocationForEntity } from '../../data/utils';
+import { entityToComponent, findLocationForEntityMeta } from '../../data/utils';
 import {
   CatalogFilter,
   CatalogFilterItem,
@@ -69,16 +69,18 @@ const CatalogPage: FC<{}> = () => {
   const styles = useStyles();
 
   const actions = [
-    (rowData: Component) => ({
-      icon: GitHub,
-      tooltip: 'View on GitHub',
-      onClick: () => {
-        if (!rowData || !rowData.location) return;
-        window.open(rowData.location.target, '_blank');
-      },
-      hidden:
-        rowData && rowData.location ? rowData.location.type !== 'github' : true,
-    }),
+    (rowData: Component) => {
+      const location = findLocationForEntityMeta(rowData.metadata);
+      return {
+        icon: GitHub,
+        tooltip: 'View on GitHub',
+        onClick: () => {
+          if (!location) return;
+          window.open(location.target, '_blank');
+        },
+        hidden: location ? location?.type !== 'github' : true,
+      };
+    },
   ];
 
   // TODO: replace me with the proper tabs implemntation
@@ -152,7 +154,7 @@ const CatalogPage: FC<{}> = () => {
                 value.map(val => {
                   return {
                     ...entityToComponent(val),
-                    locationSpec: findLocationForEntity(val),
+                    locationSpec: findLocationForEntityMeta(val.metadata),
                   };
                 })) ||
               []

plugins/catalog/src/components/CatalogTable/CatalogTable.test.tsx

@@ -23,26 +23,20 @@ const components: Component[] = [
   {
     name: 'component1',
     kind: 'Component',
+    metadata: { name: 'component1' },
     description: 'Placeholder',
-    metadata: {
-      name: 'component1',
-    },
   },
   {
     name: 'component2',
     kind: 'Component',
+    metadata: { name: 'component2' },
     description: 'Placeholder',
-    metadata: {
-      name: 'component2',
-    },
   },
   {
     name: 'component3',
     kind: 'Component',
+    metadata: { name: 'component3' },
     description: 'Placeholder',
-    metadata: {
-      name: 'component3',
-    },
   },
 ];
 
@@ -69,7 +63,7 @@ describe('CatalogTable component', () => {
       ),
     );
     const errorMessage = await rendered.findByText(
-      'Something went wrong here. Please contact #backstage for help.',
+      /Error encountered while fetching components./,
     );
     expect(errorMessage).toBeInTheDocument();
   });

plugins/catalog/src/components/CatalogTable/CatalogTable.tsx

@@ -13,8 +13,9 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import { InfoCard, Progress, Table, TableColumn } from '@backstage/core';
-import { Link, Typography } from '@material-ui/core';
+import { Progress, Table, TableColumn } from '@backstage/core';
+import { Link } from '@material-ui/core';
+import { Alert } from '@material-ui/lab';
 import React, { FC } from 'react';
 import { generatePath, Link as RouterLink } from 'react-router-dom';
 import { Component } from '../../data/component';
@@ -63,12 +64,11 @@ const CatalogTable: FC<CatalogTableProps> = ({
     return <Progress />;
   } else if (error) {
     return (
-      <InfoCard>
-        <Typography variant="subtitle1" paragraph>
-          Error encountered while fetching components.
-        </Typography>
-        <Typography>{error}</Typography>
-      </InfoCard>
+      <div>
+        <Alert severity="error">
+          Error encountered while fetching components. {error.toString()}
+        </Alert>
+      </div>
     );
   }
 

plugins/catalog/src/components/ComponentContextMenu/ComponentContextMenu.tsx

@@ -13,22 +13,24 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import React, { FC, useEffect, useRef, useState } from 'react';
 import {
   IconButton,
   ListItemIcon,
-  Menu,
   MenuItem,
+  MenuList,
+  Popover,
   Typography,
 } from '@material-ui/core';
 import Cancel from '@material-ui/icons/Cancel';
 import MoreVert from '@material-ui/icons/MoreVert';
 import SwapHoriz from '@material-ui/icons/SwapHoriz';
+import React, { FC, useState } from 'react';
 import { makeStyles } from '@material-ui/core/styles';
 
+// TODO(freben): It should probably instead be the case that Header sets the theme text color to white inside itself unconditionally instead
 const useStyles = makeStyles({
-  menu: {
-    marginTop: 52,
+  button: {
+    color: 'white',
   },
 });
 
@@ -39,52 +41,58 @@ type ComponentContextMenuProps = {
 const ComponentContextMenu: FC<ComponentContextMenuProps> = ({
   onUnregisterComponent,
 }) => {
-  const [menuOpen, setMenuOpen] = useState(false);
-  const menuAnchor = useRef<HTMLDivElement>(null);
+  const [anchorEl, setAnchorEl] = useState<HTMLButtonElement>();
   const classes = useStyles();
 
-  useEffect(() => {
-    const globalCloseHandler = (event: any) => {
-      const menu = menuAnchor.current;
-      if (menu !== null && !menu.contains(event.target)) {
-        setMenuOpen(false);
-      }
-    };
+  const onOpen = (event: React.SyntheticEvent<HTMLButtonElement>) => {
+    setAnchorEl(event.currentTarget);
+  };
 
-    window.addEventListener('click', globalCloseHandler);
-    return () => window.removeEventListener('click', globalCloseHandler);
-  }, [menuOpen]);
+  const onClose = () => {
+    setAnchorEl(undefined);
+  };
 
   return (
-    <div ref={menuAnchor}>
+    <div>
       <IconButton
         aria-label="more"
         aria-controls="long-menu"
         aria-haspopup="true"
-        onClick={() => setMenuOpen(!menuOpen)}
+        onClick={onOpen}
         data-testid="menu-button"
+        className={classes.button}
       >
         <MoreVert />
       </IconButton>
-      <Menu
-        open={menuOpen}
-        anchorEl={menuAnchor.current}
-        className={classes.menu}
+      <Popover
+        open={Boolean(anchorEl)}
+        onClose={onClose}
+        anchorEl={anchorEl}
+        anchorOrigin={{ vertical: 'bottom', horizontal: 'right' }}
+        transformOrigin={{ vertical: 'top', horizontal: 'right' }}
       >
-        <MenuItem onClick={onUnregisterComponent}>
-          <ListItemIcon>
-            <Cancel fontSize="small" />
-          </ListItemIcon>
-          <Typography variant="inherit">Unregister component</Typography>
-        </MenuItem>
-        <MenuItem>
-          <ListItemIcon>
-            <SwapHoriz fontSize="small" />
-          </ListItemIcon>
-          <Typography variant="inherit">Move repository</Typography>
-        </MenuItem>
-      </Menu>
+        <MenuList>
+          <MenuItem
+            onClick={() => {
+              onClose();
+              onUnregisterComponent();
+            }}
+          >
+            <ListItemIcon>
+              <Cancel fontSize="small" />
+            </ListItemIcon>
+            <Typography variant="inherit">Unregister component</Typography>
+          </MenuItem>
+          <MenuItem>
+            <ListItemIcon>
+              <SwapHoriz fontSize="small" />
+            </ListItemIcon>
+            <Typography variant="inherit">Move repository</Typography>
+          </MenuItem>
+        </MenuList>
+      </Popover>
     </div>
   );
 };
+
 export default ComponentContextMenu;

plugins/catalog/src/components/ComponentRemovalDialog/ComponentRemovalDialog.tsx

@@ -13,8 +13,9 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 import { Entity, LOCATION_ANNOTATION } from '@backstage/catalog-model';
-import { Progress, useApi, alertApiRef } from '@backstage/core';
+import { Progress, useApi } from '@backstage/core';
 import {
   Button,
   Dialog,
@@ -22,12 +23,14 @@ import {
   DialogContent,
   DialogContentText,
   DialogTitle,
+  Typography,
   useMediaQuery,
   useTheme,
   List,
   ListItem,
   ListItemText,
 } from '@material-ui/core';
+import Alert from '@material-ui/lab/Alert';
 import React, { FC } from 'react';
 import { useAsync } from 'react-use';
 import { AsyncState } from 'react-use/lib/useAsync';

plugins/catalog/src/data/component.ts

@@ -13,7 +13,7 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import { EntityMeta, LocationSpec } from '@backstage/catalog-model';
+import { EntityMeta } from '@backstage/catalog-model';
 import { ReactNode } from 'react';
 
 export type Component = {
@@ -21,5 +21,4 @@ export type Component = {
   kind: string;
   metadata: EntityMeta;
   description: ReactNode;
-  location?: LocationSpec;
 };

plugins/catalog/src/data/utils.tsx

@@ -13,16 +13,17 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
-import React from 'react';
-import { Component } from './component';
 import {
   Entity,
-  LOCATION_ANNOTATION,
   LocationSpec,
+  LOCATION_ANNOTATION,
+  EntityMeta,
 } from '@backstage/catalog-model';
-import Edit from '@material-ui/icons/Edit';
 import IconButton from '@material-ui/core/IconButton';
 import { styled } from '@material-ui/core/styles';
+import Edit from '@material-ui/icons/Edit';
+import React from 'react';
+import { Component } from './component';
 
 const DescriptionWrapper = styled('span')({
   display: 'flex',
@@ -39,7 +40,7 @@ const createEditLink = (location: LocationSpec): string => {
 };
 
 export function entityToComponent(envelope: Entity): Component {
-  const location = findLocationForEntity(envelope);
+  const location = findLocationForEntityMeta(envelope.metadata);
   return {
     name: envelope.metadata?.name ?? '',
     kind: envelope.kind ?? 'unknown',
@@ -56,14 +57,17 @@ export function entityToComponent(envelope: Entity): Component {
         ) : null}
       </DescriptionWrapper>
     ),
-    location: findLocationForEntity(envelope),
   };
 }
 
-export function findLocationForEntity(
-  entity: Entity,
+export function findLocationForEntityMeta(
+  meta: EntityMeta,
 ): LocationSpec | undefined {
-  const annotation = entity.metadata.annotations?.[LOCATION_ANNOTATION];
+  if (!meta) {
+    return undefined;
+  }
+
+  const annotation = meta.annotations?.[LOCATION_ANNOTATION];
   if (!annotation) {
     return undefined;
   }