Skip to content

C
coopwire-hypothesis
Commit 78d259c0 authored by Lyza Danger Gardner's avatar Lyza Danger Gardner Committed by Lyza Gardner
Browse files
Options

Migrate MenuItem to tailwind

parent 9c9656da
Hide whitespace changes
Inline Side-by-side
Showing with 191 additions and 251 deletions
src/sidebar/components/MenuItem.js
View file @ 78d259c0
import classnames from 'classnames';
import { SvgIcon, normalizeKeyName } from '@hypothesis/frontend-shared';
import { Icon, normalizeKeyName } from '@hypothesis/frontend-shared';
import { useEffect, useRef } from 'preact/hooks';
import MenuKeyboardNavigation from './MenuKeyboardNavigation';
import Slider from './Slider';
/**
* Render a clickable div that will toggle the expanded state of the
* associated submenu via `onToggleSubmenu`.
*
* @param {object} props
* @param {string} props.title
* @param {boolean} props.isExpanded
* @param {(e: Event) => void} [props.onToggleSubmenu]
*/
function SubmenuToggle({ title, isExpanded, onToggleSubmenu }) {
return (
<div
data-testid="submenu-toggle"
// We should not have a <button> inside of the menu item itself
// but we have a non-standard mechanism with the toggle control
// requiring an onClick event nested inside a "menuitemradio|menuitem".
// Therefore, a static element with a role="none" is necessary here.
role="none"
className={classnames(
// Center content in a 40px square. The entire element is clickable
'flex flex-col items-center justify-center w-10 h-10',
'text-grey-6 bg-grey-1',
// Clip the background (color) such that it only shows within the
// content box, which is a 24px rounded square formed by the large
// borders
'bg-clip-content border-[8px] border-transparent rounded-xl',
// When the menu item is hovered AND this element is hovered, darken
// the text color so it is clear that the toggle is the hovered element
'group-hover:hover:text-grey-8',
{
// When the submenu is expanded, this element always has a darker
// background color regardless of hover state.
'bg-grey-4': isExpanded,
// When the parent menu item is hovered, it gets a darker background.
// Make the toggle background darker also.
'group-hover:bg-grey-3': !isExpanded,
}
)}
onClick={onToggleSubmenu}
title={title}
>
<Icon
name={isExpanded ? 'collapse-menu' : 'expand-menu'}
classes="w-3 h-3"
/>
</div>
);
}
/**
* @typedef MenuItemProps
* @prop {string} [href] -
......@@ -75,29 +124,14 @@ export default function MenuItem({
onToggleSubmenu,
submenu,
}) {
const iconClass = 'MenuItem__icon';
const iconIsUrl = icon && icon.indexOf('/') !== -1;
const hasLeftIcon = icon || isSubmenuItem;
const hasRightIcon = icon && isSubmenuItem;
const menuItemRef =
/** @type {{ current: HTMLAnchorElement & HTMLDivElement }} */ (useRef());
/** @type {number|undefined} */
let focusTimer;
let renderedIcon = null;
if (icon && icon !== 'blank') {
renderedIcon = iconIsUrl ? (
<img className={iconClass} alt={iconAlt} src={icon} />
) : (
<SvgIcon name={icon} className="MenuItem__icon" />
);
}
const leftIcon = isSubmenuItem ? null : renderedIcon;
const rightIcon = isSubmenuItem ? renderedIcon : null;
// menuItem can be either a link or a button
let menuItem;
const hasSubmenuVisible = typeof isSubmenuVisible === 'boolean';
......@@ -140,15 +174,96 @@ export default function MenuItem({
}
}
};
let renderedIcon = null;
if (icon && icon !== 'blank') {
renderedIcon = iconIsUrl ? (
<img className="w-4 h-4" alt={iconAlt} src={icon} />
) : (
<Icon name={icon} classes="h-3 w-3" />
);
}
const leftIcon = isSubmenuItem ? null : renderedIcon;
const rightIcon = isSubmenuItem ? renderedIcon : null;
// MenuItem content layout consists of:
// - Sometimes a left item, which may contain an icon or serve as
// an indenting space for label alignment
// - Always a label
// - Sometimes a right item, which contains an icon (submenu items)
// - Sometimes a submenu-toggle control (only if the item has a submenu)
const hasLeftItem = leftIcon || isSubmenuItem || icon === 'blank';
const hasRightItem = rightIcon && isSubmenuItem;
const menuItemContent = (
<>
{hasLeftItem && (
<div
className="w-7 flex items-center justify-center"
data-testid="left-item-container"
>
{leftIcon}
</div>
)}
<span className="flex items-center grow whitespace-nowrap px-1">
{label}
</span>
{hasRightItem && (
<div
className="w-8 flex items-center justify-center"
data-testid="right-item-container"
>
{rightIcon}
</div>
)}
{hasSubmenuVisible && (
<SubmenuToggle
title={`Show actions for ${label}`}
isExpanded={isSubmenuVisible}
onToggleSubmenu={onToggleSubmenu}
/>
)}
</>
);
const wrapperClasses = classnames(
'hyp-u-outline-on-keyboard-focus--inset',
'w-full min-w-[150px] flex items-center select-none',
'border-b',
// Set this container as a "group" so that children may style based on its
// layout state.
// See https://tailwindcss.com/docs/hover-focus-and-other-states#styling-based-on-parent-state
'group',
{
'min-h-[30px] font-normal': isSubmenuItem,
'min-h-[40px] font-medium': !isSubmenuItem,
'bg-grey-1 hover:bg-grey-3': isSubmenuItem || isExpanded,
'bg-white hover:bg-grey-1': !isSubmenuItem && !isExpanded,
// visual "padding" on the right is part of SubmenuToggle when rendered,
// but when not rendering a SubmenuToggle, we need to add some padding here
'pr-1': !hasSubmenuVisible,
},
{
// When the item is selected, show a left border to indicate it
'border-l-[4px] border-l-brand': isSelected,
// Add equivalent padding to border size when not selected. This instead
// of a transparent left border to make focus ring cover the full
// menu item. Otherwise the focus ring will be inset on the left too far.
'pl-[4px]': !isSelected,
'border-b-grey-3': isExpanded,
'border-b-transparent': !isExpanded,
'text-color-text-light': isDisabled,
'text-color-text': !isDisabled,
}
);
if (href) {
// The menu item is a link
menuItem = (
<a
ref={menuItemRef}
className={classnames('MenuItem', {
'is-submenu': isSubmenuItem,
'is-disabled': isDisabled,
})}
className={wrapperClasses}
data-testid="menu-item"
href={href}
target="_blank"
tabIndex={-1}
......@@ -156,28 +271,17 @@ export default function MenuItem({
role="menuitem"
onKeyDown={onKeyDown}
>
{hasLeftIcon && (
<div className="MenuItem__icon-container">{leftIcon}</div>
)}
<span className="MenuItem__label">{label}</span>
{hasRightIcon && (
<div className="MenuItem__icon-container">{rightIcon}</div>
)}
{menuItemContent}
</a>
);
} else {
// The menu item is a clickable button or radio button.
// In either case there may be an optional submenu.
menuItem = (
<div
ref={menuItemRef}
className={classnames('MenuItem', {
'is-submenu': isSubmenuItem,
'is-disabled': isDisabled,
'is-expanded': isExpanded,
'is-selected': isSelected,
})}
className={wrapperClasses}
data-testid="menu-item"
tabIndex={-1}
onKeyDown={onKeyDown}
onClick={onClick}
......@@ -186,32 +290,7 @@ export default function MenuItem({
aria-haspopup={hasSubmenuVisible}
aria-expanded={hasSubmenuVisible ? isSubmenuVisible : undefined}
>
{hasLeftIcon && (
<div className="MenuItem__icon-container">{leftIcon}</div>
)}
<span className="MenuItem__label">{label}</span>
{hasRightIcon && (
<div className="MenuItem__icon-container">{rightIcon}</div>
)}
{hasSubmenuVisible && (
<div
// We should not have a <button> inside of the menu item itself
// but we have a non-standard mechanism with the toggle control
// requiring an onClick event nested inside a "menuitemradio|menuitem".
// Therefore, a static element with a role="none" is necessary here.
role="none"
icon={isSubmenuVisible ? 'collapse-menu' : 'expand-menu'}
className="MenuItem__toggle"
onClick={onToggleSubmenu}
title={`Show actions for ${label}`}
>
<SvgIcon
name={isSubmenuVisible ? 'collapse-menu' : 'expand-menu'}
className="MenuItem__toggle-icon"
/>
</div>
)}
{menuItemContent}
</div>
);
}
......@@ -223,7 +302,7 @@ export default function MenuItem({
<MenuKeyboardNavigation
closeMenu={onCloseSubmenu}
visible={/** @type {boolean} */ (isSubmenuVisible)}
className="MenuItem__submenu"
className="border-b"
>
{submenu}
</MenuKeyboardNavigation>
......
src/sidebar/components/test/MenuItem-test.js
View file @ 78d259c0
......@@ -17,6 +17,8 @@ describe('MenuItem', () => {
});
};
const menuItemSelector = '[data-testid="menu-item"]';
beforeEach(() => {
$imports.$mock(mockImportedComponents());
});
......@@ -48,9 +50,9 @@ describe('MenuItem', () => {
it('invokes `onClick` callback when pressing `Enter` or space', () => {
const onClick = sinon.stub();
const wrapper = createMenuItem({ href: 'https://example.com', onClick });
wrapper.find('.MenuItem').simulate('keydown', { key: 'Enter' });
wrapper.find(menuItemSelector).simulate('keydown', { key: 'Enter' });
assert.called(onClick);
wrapper.find('.MenuItem').simulate('keydown', { key: ' ' });
wrapper.find(menuItemSelector).simulate('keydown', { key: ' ' });
assert.calledTwice(onClick);
});
});
......@@ -59,60 +61,66 @@ describe('MenuItem', () => {
it('renders a non-link if an `href` is not provided', () => {
const wrapper = createMenuItem();
assert.isFalse(wrapper.exists('a'));
assert.equal(wrapper.find('.MenuItem__label').text(), 'Test item');
assert.equal(wrapper.find(menuItemSelector).text(), 'Test item');
});
it('invokes `onClick` callback when clicked', () => {
const onClick = sinon.stub();
const wrapper = createMenuItem({ onClick });
wrapper.find('.MenuItem').simulate('click');
wrapper.find(menuItemSelector).simulate('click');
assert.called(onClick);
});
it('invokes `onClick` callback when pressing `Enter` or space', () => {
const onClick = sinon.stub();
const wrapper = createMenuItem({ isSelected: true, onClick });
wrapper.find('.MenuItem').simulate('keydown', { key: 'Enter' });
wrapper.find(menuItemSelector).simulate('keydown', { key: 'Enter' });
assert.called(onClick);
wrapper.find('.MenuItem').simulate('keydown', { key: ' ' });
wrapper.find(menuItemSelector).simulate('keydown', { key: ' ' });
assert.calledTwice(onClick);
});
it('has proper aria attributes when `isSelected` is a boolean', () => {
const wrapper = createMenuItem({ isSelected: true });
assert.equal(wrapper.find('.MenuItem').prop('role'), 'menuitemradio');
assert.equal(wrapper.find('.MenuItem').prop('aria-checked'), true);
assert.equal(
wrapper.find(menuItemSelector).prop('role'),
'menuitemradio'
);
assert.equal(wrapper.find(menuItemSelector).prop('aria-checked'), true);
// aria-haspopup should be false without a submenu
assert.equal(wrapper.find('.MenuItem').prop('aria-haspopup'), false);
assert.equal(wrapper.find(menuItemSelector).prop('aria-haspopup'), false);
});
});
describe('icons', () => {
it('renders an SVG icon if an icon name is provided', () => {
describe('icons for top-level menu items', () => {
it('renders an icon if an icon name is provided', () => {
const wrapper = createMenuItem({ icon: 'edit' });
assert.isTrue(wrapper.exists('SvgIcon[name="edit"]'));
assert.isTrue(wrapper.exists('Icon[name="edit"]'));
});
it('renders a blank space for an icon if `icon` is "blank"', () => {
it('adds a left container if `icon` is "blank"', () => {
const wrapper = createMenuItem({ icon: 'blank' });
const iconSpace = wrapper.find('.MenuItem__icon-container');
assert.equal(iconSpace.length, 1);
assert.equal(iconSpace.children().length, 0);
assert.equal(
wrapper.find('[data-testid="left-item-container"]').length,
1
);
});
it('does not render a space for an icon if `icon` is missing', () => {
it('does not add a left container if `icon` is missing', () => {
const wrapper = createMenuItem();
const iconSpace = wrapper.find('.MenuItem__icon-container');
assert.equal(iconSpace.length, 0);
assert.equal(
wrapper.find('[data-testid="left-item-container"]').length,
0
);
});
it('renders top-level menu item icons on the left', () => {
it('renders an icon on the left if `icon` provided', () => {
const wrapper = createMenuItem({ icon: 'edit' });
const iconSpaces = wrapper.find('.MenuItem__icon-container');
const leftItem = wrapper.find('[data-testid="left-item-container"]');
// There should be only one icon space, on the left.
assert.equal(iconSpaces.length, 1);
assert.equal(iconSpaces.at(0).children().length, 1);
assert.equal(leftItem.length, 1);
assert.equal(leftItem.at(0).children().length, 1);
});
});
......@@ -122,21 +130,24 @@ describe('MenuItem', () => {
isSubmenuVisible: true,
submenu: <div role="menuitem">Submenu content</div>,
});
assert.isTrue(wrapper.exists('SvgIcon[name="collapse-menu"]'));
assert.equal(wrapper.find('.MenuItem').prop('aria-expanded'), true);
assert.isTrue(wrapper.exists('SubmenuToggle'));
assert.equal(wrapper.find(menuItemSelector).prop('aria-expanded'), true);
wrapper.setProps({ isSubmenuVisible: false });
assert.isTrue(wrapper.exists('SvgIcon[name="expand-menu"]'));
assert.equal(wrapper.find('.MenuItem').prop('aria-haspopup'), true);
assert.equal(wrapper.find('.MenuItem').prop('aria-expanded'), false);
assert.isNotOk(wrapper.find('.MenuItem').prop('aria-expanded'));
assert.isTrue(wrapper.exists('Icon[name="expand-menu"]'));
assert.equal(wrapper.find(menuItemSelector).prop('aria-haspopup'), true);
assert.equal(wrapper.find(menuItemSelector).prop('aria-expanded'), false);
assert.isNotOk(wrapper.find(menuItemSelector).prop('aria-expanded'));
});
it('does not show submenu indicator if `isSubmenuVisible` is undefined', () => {
const wrapper = createMenuItem();
assert.isFalse(wrapper.exists('SvgIcon'));
assert.isFalse(wrapper.exists('Icon'));
// aria-expanded should be undefined
assert.equal(wrapper.find('.MenuItem').prop('aria-expanded'), undefined);
assert.equal(
wrapper.find(menuItemSelector).prop('aria-expanded'),
undefined
);
});
it('calls the `onToggleSubmenu` callback when the submenu toggle is clicked', () => {
......@@ -146,7 +157,7 @@ describe('MenuItem', () => {
onToggleSubmenu: fakeOnToggleSubmenu,
submenu: <div role="menuitem">Submenu content</div>,
});
wrapper.find('.MenuItem__toggle').simulate('click');
wrapper.find('[data-testid="submenu-toggle"]').simulate('click');
assert.called(fakeOnToggleSubmenu);
});
......@@ -157,7 +168,7 @@ describe('MenuItem', () => {
onToggleSubmenu: fakeOnToggleSubmenu,
submenu: <div role="menuitem">Item</div>,
});
wrapper.find('.MenuItem').simulate('keydown', { key: 'ArrowRight' });
wrapper.find(menuItemSelector).simulate('keydown', { key: 'ArrowRight' });
assert.called(fakeOnToggleSubmenu);
});
......@@ -167,14 +178,12 @@ describe('MenuItem', () => {
isSubmenuItem: true,
submenu: <div role="menuitem">Submenu content</div>,
});
const iconSpaces = wrapper.find('.MenuItem__icon-container');
assert.equal(iconSpaces.length, 2);
const rightItem = wrapper.find('[data-testid="right-item-container"]');
// There should be a space for the parent item's icon.
assert.equal(iconSpaces.at(0).children().length, 0);
assert.equal(rightItem.length, 1);
// The actual icon for the submenu should be shown on the right.
assert.equal(iconSpaces.at(1).children().length, 1);
assert.equal(rightItem.at(0).children().length, 1);
});
it('does not render submenu content if `isSubmenuVisible` is undefined', () => {
......@@ -241,7 +250,10 @@ describe('MenuItem', () => {
.closeMenu({ key: 'Enter' });
});
clock.tick(1);
assert.equal(document.activeElement.className, 'MenuItem');
assert.equal(
document.activeElement.getAttribute('data-testid'),
'menu-item'
);
} finally {
clock.restore();
}
......
src/styles/sidebar/components/MenuItem.scss deleted 100644 → 0
View file @ 9c9656da
@use '@hypothesis/frontend-shared/styles/mixins/focus';
@use 'sass:color';
@use '../../mixins/layout';
@use '../../mixins/utils';
@use '../../variables' as var;
$MenuItem-padding: 10px;
$item-height: 40px;
// The container for the clickable area of the menu item which triggers an
// action or link. For menu items without submenus, this covers the full area of
// the menu item. For menu items with submenus, this covers the full area
// except for the toggle button that opens the submenu.
a.MenuItem:hover {
color: var.$color-link-hover;
}
.MenuItem {
@include focus.outline-on-keyboard-focus($inset: true);
@include layout.row($align: center);
align-self: stretch;
flex-grow: 1;
appearance: none;
padding-left: $MenuItem-padding;
color: var.$color-text;
border: none;
padding-right: 0;
font-weight: 500;
min-height: 40px;
width: 100%;
min-width: 150px;
// Prevent height changes when .is-expanded border is rendered.
border-bottom: solid 1px transparent;
// Prevent menu item text from being selected when user toggles menu.
user-select: none;
&:hover {
background-color: var.$grey-1;
.MenuItem__toggle {
// Make it a bit darker when its a submenu item.
background-color: var.$grey-3;
}
}
&.is-submenu {
min-height: $item-height - 10px;
background: var.$grey-1;
color: var.$color-text;
font-weight: normal;
&:hover {
background-color: var.$grey-3;
}
}
&.is-selected {
$border-width: 4px;
border-left: $border-width solid var.$color-brand;
padding-left: $MenuItem-padding - $border-width;
}
&.is-expanded {
@include utils.border--bottom;
background-color: var.$grey-1;
color: var.$color-text;
&:hover {
background-color: var.$grey-3;
}
.MenuItem__toggle {
// Make it a bit darker when its expanded. This is needed for
// a color darker than hover while expanded.
background-color: var.$grey-4;
}
}
&.is-disabled {
color: var.$color-text--light;
}
}
.MenuItem__icon {
@include utils.icon--medium;
color: inherit;
margin-right: 4px;
position: relative;
&-container {
margin-right: 10px;
// TODO Not using icon mixins for now until reviewing this entire pattern
width: 16px;
height: 16px;
}
}
.MenuItem__label {
@include layout.row($align: center);
align-self: stretch;
color: inherit;
white-space: nowrap;
flex-grow: 1;
font-weight: inherit;
padding-right: $MenuItem-padding;
&--submenu {
font-weight: normal;
}
}
// Toggle button used to expand or collapse the submenu associated with a menu
// item.
.MenuItem__toggle {
@include layout.column(center, center);
align-self: stretch;
width: 40px;
padding: 0;
height: 40px;
color: var.$grey-5;
// Add a wide transparent border to provide a large-enough hit target (~40px),
// larger than the visual size of the button (~20px).
background-color: var.$grey-1;
background-clip: content-box;
border: 7px solid transparent;
// Add slight rounded borders. border-radius sets the outer radius, but
// what the user sees is the inner radius, which is much smaller.
border-radius: 12px;
&:hover {
background-color: var.$grey-4;
color: var.$grey-6;
}
&-icon {
// TODO not an icon in the general sense of the term; rename class?
width: 12px;
height: 12px;
}
}
// The container for open submenus
.MenuItem__submenu {
@include utils.border--bottom;
&:hover {
// Make it a bit darker on hover.
background-color: var.$grey-3;
}
}
src/styles/sidebar/components/_index.scss
View file @ 78d259c0
......@@ -13,7 +13,6 @@
@use './GroupList';
@use './GroupListItem';
@use './Menu';
@use './MenuItem';
@use './MenuSection';
@use './StyledText';
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment