diff --git a/.changeset/rich-games-yawn.md b/.changeset/rich-games-yawn.md new file mode 100644 index 0000000000..1106a8b3de --- /dev/null +++ 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 new file mode 100644 index 0000000000..099a3477d3 --- /dev/null +++ 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( + + + {}} + text="Create..." + /> + , + ); + 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 index 929828cee4..f09ac1a6a0 100644 --- a/packages/core/src/layout/Sidebar/Items.tsx +++ b/packages/core/src/layout/Sidebar/Items.tsx @@ -53,6 +53,15 @@ const useStyles = makeStyles(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(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( - ( - { 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 = ( - - - - ); +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 ( -
- {itemIcon} -
- ); - } +function isButtonItem( + props: SidebarItemProps, +): props is SidebarItemButtonProps { + return (props as SidebarItemLinkProps).to === undefined; +} - return ( - - {itemIcon} - - ); - } +export const SidebarItem = forwardRef((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 = ( - <> -
- {itemIcon} -
- {text && ( - - {text} - - )} -
{children}
- - ); + const itemIcon = ( + + + + ); - if (to === undefined) { - return ( -
- {content} -
- ); - } + const closedContent = itemIcon; + const openContent = ( + <> +
+ {itemIcon} +
+ {text && ( + + {text} + + )} +
{children}
+ + ); + + 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 ( - + ); - }, -); + } + + return ( + + {content} + + ); +}); type SidebarSearchFieldProps = { onSearch: (input: string) => void;