fix(core): SidebarItem without to prop renders accessible button

backstage · Dec 24, 2020 · 265a7ab · 265a7ab
1 parent 8509fc2
commit 265a7ab
Show file tree

Hide file tree

Showing 3 changed files with 153 additions and 78 deletions.
diff --git a/.changeset/rich-games-yawn.md b/.changeset/rich-games-yawn.md
@@ -0,0 +1,5 @@
+---
+'@backstage/core': patch
+---
+
+Fix issue where `SidebarItem` with `onClick` and without `to` renders an inaccessible div. It now renders a button.
diff --git a/packages/core/src/layout/Sidebar/Items.test.tsx b/packages/core/src/layout/Sidebar/Items.test.tsx
@@ -0,0 +1,58 @@
+/*
+ * Copyright 2020 Spotify AB
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import React from 'react';
+import { renderInTestApp } from '@backstage/test-utils';
+import { screen } from '@testing-library/react';
+import userEvent from '@testing-library/user-event';
+import HomeIcon from '@material-ui/icons/Home';
+import CreateComponentIcon from '@material-ui/icons/AddCircleOutline';
+import { Sidebar } from './Bar';
+import { SidebarItem } from './Items';
+
+async function renderSidebar() {
+  await renderInTestApp(
+    <Sidebar>
+      <SidebarItem text="Home" icon={HomeIcon} to="./" />
+      <SidebarItem
+        icon={CreateComponentIcon}
+        onClick={() => {}}
+        text="Create..."
+      />
+    </Sidebar>,
+  );
+  userEvent.hover(screen.getByTestId('sidebar-root'));
+}
+
+describe('Items', () => {
+  beforeEach(async () => {
+    await renderSidebar();
+  });
+
+  describe('SidebarItem', () => {
+    it('should render a link when `to` prop provided', async () => {
+      expect(
+        await screen.findByRole('link', { name: /home/i }),
+      ).toBeInTheDocument();
+    });
+
+    it('should render a button when `to` prop is not provided', async () => {
+      expect(
+        await screen.findByRole('button', { name: /create/i }),
+      ).toBeInTheDocument();
+    });
+  });
+});
diff --git a/packages/core/src/layout/Sidebar/Items.tsx b/packages/core/src/layout/Sidebar/Items.tsx
@@ -53,6 +53,15 @@ const useStyles = makeStyles<BackstageTheme>(theme => {
       height: 48,
       cursor: 'pointer',
     },
+    buttonItem: {
+      background: 'none',
+      border: 'none',
+      width: 'auto',
+      margin: 0,
+      padding: 0,
+      textAlign: 'inherit',
+      font: 'inherit',
+    },
     closed: {
       width: drawerWidthClosed,
       justifyContent: 'center',
@@ -114,100 +123,103 @@ const useStyles = makeStyles<BackstageTheme>(theme => {
   };
 });
 
-type SidebarItemProps = {
+type SidebarItemBaseProps = {
   icon: IconComponent;
   text?: string;
-  // If 'to' is set the item will act as a nav link with highlight, otherwise it's just a button
-  to?: string;
   hasNotifications?: boolean;
-  onClick?: (ev: React.MouseEvent) => void;
   children?: ReactNode;
 };
 
-export const SidebarItem = forwardRef<any, SidebarItemProps>(
-  (
-    { icon: Icon, text, to, hasNotifications = false, onClick, children },
-    ref,
-  ) => {
-    const classes = useStyles();
-    // XXX (@koroeskohr): unsure this is optimal. But I just really didn't want to have the item component
-    // depend on the current location, and at least have it being optionally forced to selected.
-    // Still waiting on a Q answered to fine tune the implementation
-    const { isOpen } = useContext(SidebarContext);
+type SidebarItemButtonProps = SidebarItemBaseProps & {
+  onClick: (ev: React.MouseEvent) => void;
+};
 
-    const itemIcon = (
-      <Badge
-        color="secondary"
-        variant="dot"
-        overlap="circle"
-        invisible={!hasNotifications}
-      >
-        <Icon fontSize="small" className={classes.icon} />
-      </Badge>
-    );
+type SidebarItemLinkProps = SidebarItemBaseProps & {
+  text?: string;
+  to: string;
+  onClick?: (ev: React.MouseEvent) => void;
+};
 
-    const childProps = {
-      onClick,
-      className: clsx(classes.root, isOpen ? classes.open : classes.closed),
-    };
+type SidebarItemProps = SidebarItemButtonProps | SidebarItemLinkProps;
 
-    if (!isOpen) {
-      if (to === undefined) {
-        return (
-          <div {...childProps} ref={ref}>
-            {itemIcon}
-          </div>
-        );
-      }
+function isButtonItem(
+  props: SidebarItemProps,
+): props is SidebarItemButtonProps {
+  return (props as SidebarItemLinkProps).to === undefined;
+}
 
-      return (
-        <NavLink
-          {...childProps}
-          activeClassName={classes.selected}
-          to={to}
-          end
-          ref={ref}
-        >
-          {itemIcon}
-        </NavLink>
-      );
-    }
+export const SidebarItem = forwardRef<any, SidebarItemProps>((props, ref) => {
+  const {
+    icon: Icon,
+    text,
+    hasNotifications = false,
+    onClick,
+    children,
+  } = props;
+  const classes = useStyles();
+  // XXX (@koroeskohr): unsure this is optimal. But I just really didn't want to have the item component
+  // depend on the current location, and at least have it being optionally forced to selected.
+  // Still waiting on a Q answered to fine tune the implementation
+  const { isOpen } = useContext(SidebarContext);
 
-    const content = (
-      <>
-        <div data-testid="login-button" className={classes.iconContainer}>
-          {itemIcon}
-        </div>
-        {text && (
-          <Typography variant="subtitle2" className={classes.label}>
-            {text}
-          </Typography>
-        )}
-        <div className={classes.secondaryAction}>{children}</div>
-      </>
-    );
+  const itemIcon = (
+    <Badge
+      color="secondary"
+      variant="dot"
+      overlap="circle"
+      invisible={!hasNotifications}
+    >
+      <Icon fontSize="small" className={classes.icon} />
+    </Badge>
+  );
 
-    if (to === undefined) {
-      return (
-        <div {...childProps} ref={ref}>
-          {content}
-        </div>
-      );
-    }
+  const closedContent = itemIcon;
+
+  const openContent = (
+    <>
+      <div data-testid="login-button" className={classes.iconContainer}>
+        {itemIcon}
+      </div>
+      {text && (
+        <Typography variant="subtitle2" className={classes.label}>
+          {text}
+        </Typography>
+      )}
+      <div className={classes.secondaryAction}>{children}</div>
+    </>
+  );
+
+  const content = isOpen ? openContent : closedContent;
 
+  const childProps = {
+    onClick,
+    className: clsx(
+      classes.root,
+      isOpen ? classes.open : classes.closed,
+      isButtonItem(props) && classes.buttonItem,
+    ),
+  };
+
+  if (isButtonItem(props)) {
     return (
-      <NavLink
-        {...childProps}
-        activeClassName={classes.selected}
-        to={to}
-        end
-        ref={ref}
-      >
+      <button {...childProps} ref={ref}>
         {content}
-      </NavLink>
+      </button>
     );
-  },
-);
+  }
+
+  return (
+    <NavLink
+      {...childProps}
+      activeClassName={classes.selected}
+      to={props.to}
+      end
+      ref={ref}
+    >
+      {content}
+    </NavLink>
+  );
+});
 
 type SidebarSearchFieldProps = {
   onSearch: (input: string) => void;