Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make highlight Actions menu items (Edit and Delete) buttons #2332

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions src/app/components/Dropdown.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ describe('Dropdown', () => {
const useOnEscSpy = jest.spyOn(reactUtils, 'useOnEsc');

const focus = jest.fn();
const focus2 = jest.fn();
const addEventListener = jest.fn();
const removeEventListener = jest.fn();
const createNodeMock = () => ({focus, addEventListener, removeEventListener});
Expand All @@ -126,12 +127,19 @@ describe('Dropdown', () => {

expect(() => component.root.findByType(DropdownList)).not.toThrow();

renderer.act(() => {
const buttons = component.root.findAllByType('button');

buttons[1].props.onMouseEnter({target: {focus: focus2}});
});

renderer.act(() => {
useOnEscSpy.mock.calls[0][1]();
});

expect(() => component.root.findByType(DropdownList)).toThrow();
expect(focus).toHaveBeenCalled();
expect(focus2).toHaveBeenCalled();

useOnEscSpy.mockClear();
});
Expand All @@ -149,9 +157,10 @@ describe('Dropdown', () => {
</TestContainer>);

renderer.act(() => {
const [button1, button2] = component.root.findAllByType('a');
button1.props.onClick(mockEv);
button2.props.onClick(mockEv);
const items = component.root.findAll((i) => i.props.onClick && i.type === 'button');

items.forEach((i) => i.props.onClick(mockEv));
expect(items.length).toBe(2);
});

expect(mockEv.preventDefault).toHaveBeenCalledTimes(2);
Expand Down
57 changes: 37 additions & 20 deletions src/app/components/Dropdown.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@

import { HTMLElement } from '@openstax/types/lib.dom';
import { HTMLElement, HTMLMenuElement } from '@openstax/types/lib.dom';
import flow from 'lodash/fp/flow';
import isUndefined from 'lodash/fp/isUndefined';
import omitBy from 'lodash/fp/omitBy';
import React, { ReactNode } from 'react';
import { FormattedMessage, useIntl } from 'react-intl';
import styled, { css, keyframes } from 'styled-components/macro';
import { useFocusLost, useTrapTabNavigation } from '../reactUtils';
import { useFocusLost, useTrapTabNavigation, focusableItemQuery } from '../reactUtils';
import { useOnEsc } from '../reactUtils';
import theme, { defaultFocusOutline } from '../theme';
import { preventDefault } from '../utils';
Expand Down Expand Up @@ -167,18 +167,28 @@ const TabTransparentDropdown = styled((
`;

function TrappingDropdownList(props: object) {
const ref = React.useRef(null);
const ref = React.useRef<HTMLMenuElement>(null);

useTrapTabNavigation(ref);

React.useEffect(
() => {
if (ref.current?.querySelector) {
ref.current?.querySelector<HTMLElement>(focusableItemQuery)?.focus();
}
},
[]
);

return (
<ol ref={ref} {...props} />
<menu ref={ref} {...props} />
);
}


// tslint:disable-next-line:variable-name
export const DropdownList = styled(TrappingDropdownList)`
list-style: none;
margin: 0;
padding: 0.6rem 0;
background: ${theme.color.neutral.formBackground};
Expand Down Expand Up @@ -207,7 +217,6 @@ export const DropdownList = styled(TrappingDropdownList)`
font-size: 1.4rem;
line-height: 2rem;

&:hover,
&:focus {
background: ${theme.color.neutral.formBorder};
${defaultFocusOutline}
Expand Down Expand Up @@ -235,24 +244,32 @@ const DropdownItemContent = ({
'data-analytics-label': dataAnalyticsLabel,
'data-analytics-region': dataAnalyticsRegion,
});
return <FormattedMessage id={message}>
const focusMe = React.useCallback(
({target: me}) => me.focus(),
[]
);

return <FormattedMessage id={message}>
{(msg) => href
? <a href={href} tabIndex={0} onClick={onClick} target={target} {...analyticsDataProps}>{prefix}{msg}</a>
/*
this should be a button but Safari and firefox don't support focusing buttons
which breaks the tab transparent dropdown
https://bugs.webkit.org/show_bug.cgi?id=22261
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus
*/
// eslint-disable-next-line jsx-a11y/anchor-is-valid
: <a
? <a
role='button'
href={href}
tabIndex={0}
onClick={onClick}
target={target}
onMouseEnter={focusMe}
{...analyticsDataProps}
>{prefix}{msg}</a>
// Safari support tab-navigation of buttons; this operates with space or Enter
: <button
type='button'
tabIndex={0}
href=''
onClick={onClick ? flow(preventDefault, onClick) : preventDefault}
onMouseEnter={focusMe}
{...analyticsDataProps}
>
{prefix}{msg}
</a>
</button>
}
</FormattedMessage>;
};
Expand All @@ -261,9 +278,9 @@ const DropdownItemContent = ({
export const DropdownItem = ({ariaMessage, ...contentProps}: DropdownItemProps) => {
const intl = useIntl();

return ariaMessage
? <li aria-label={intl.formatMessage({id: ariaMessage})}><DropdownItemContent {...contentProps}/></li>
: <li><DropdownItemContent {...contentProps} /></li>;
return <li aria-label={ariaMessage ? intl.formatMessage({id: ariaMessage}) : undefined}>
<DropdownItemContent {...contentProps}/>
</li>;
};

interface CommonDropdownProps {
Expand Down
32 changes: 18 additions & 14 deletions src/app/components/__snapshots__/DotMenu.spec.tsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,7 @@ exports[`Dropdown matches snapshot 1`] = `
}

.c12 {
list-style: none;
margin: 0;
padding: 0.6rem 0;
background: #f5f5f5;
Expand Down Expand Up @@ -173,8 +174,6 @@ exports[`Dropdown matches snapshot 1`] = `
line-height: 2rem;
}

.c12 li button:hover,
.c12 li a:hover,
.c12 li button:focus,
.c12 li a:focus {
background: #d5d5d5;
Expand Down Expand Up @@ -224,28 +223,31 @@ exports[`Dropdown matches snapshot 1`] = `
</svg>
</div>
</button>
<ol
<menu
className="c12"
>
<li>
<a
href=""
<button
onClick={[Function]}
onMouseEnter={[Function]}
tabIndex={0}
type="button"
>
Delete
</a>
</button>
</li>
<li>
<a
href="/wooo"
onClick={[Function]}
onMouseEnter={[Function]}
role="button"
tabIndex={0}
>
Edit
</a>
</li>
</ol>
</menu>
</div>
<button
aria-label="Actions"
Expand Down Expand Up @@ -409,6 +411,7 @@ exports[`Dropdown matches snapshot on right align 1`] = `
}

.c12 {
list-style: none;
margin: 0;
padding: 0.6rem 0;
background: #f5f5f5;
Expand Down Expand Up @@ -446,8 +449,6 @@ exports[`Dropdown matches snapshot on right align 1`] = `
line-height: 2rem;
}

.c12 li button:hover,
.c12 li a:hover,
.c12 li button:focus,
.c12 li a:focus {
background: #d5d5d5;
Expand Down Expand Up @@ -497,29 +498,32 @@ exports[`Dropdown matches snapshot on right align 1`] = `
</svg>
</div>
</button>
<ol
<menu
className="c12"
rightAlign={true}
>
<li>
<a
href=""
<button
onClick={[Function]}
onMouseEnter={[Function]}
tabIndex={0}
type="button"
>
Delete
</a>
</button>
</li>
<li>
<a
href="/wooo"
onClick={[Function]}
onMouseEnter={[Function]}
role="button"
tabIndex={0}
>
Edit
</a>
</li>
</ol>
</menu>
</div>
<button
aria-label="Actions"
Expand Down
32 changes: 18 additions & 14 deletions src/app/components/__snapshots__/Dropdown.spec.tsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ exports[`Dropdown matches snapshot 1`] = `
}

.c6 {
list-style: none;
margin: 0;
padding: 0.6rem 0;
background: #f5f5f5;
Expand Down Expand Up @@ -129,8 +130,6 @@ exports[`Dropdown matches snapshot 1`] = `
line-height: 2rem;
}

.c6 li button:hover,
.c6 li a:hover,
.c6 li button:focus,
.c6 li a:focus {
background: #d5d5d5;
Expand All @@ -155,28 +154,31 @@ exports[`Dropdown matches snapshot 1`] = `
>
show more
</button>
<ol
<menu
className="c6"
>
<li>
<a
href=""
<button
onClick={[Function]}
onMouseEnter={[Function]}
tabIndex={0}
type="button"
>
Delete
</a>
</button>
</li>
<li>
<a
href="/wooo"
onClick={[Function]}
onMouseEnter={[Function]}
role="button"
tabIndex={0}
>
Edit
</a>
</li>
</ol>
</menu>
</div>
<button
className="c4 c5"
Expand Down Expand Up @@ -400,6 +402,7 @@ exports[`Dropdown matches snapshot for tab hidden (open) 1`] = `
}

.c4 {
list-style: none;
margin: 0;
padding: 0.6rem 0;
background: #f5f5f5;
Expand Down Expand Up @@ -437,8 +440,6 @@ exports[`Dropdown matches snapshot for tab hidden (open) 1`] = `
line-height: 2rem;
}

.c4 li button:hover,
.c4 li a:hover,
.c4 li button:focus,
.c4 li a:focus {
background: #d5d5d5;
Expand All @@ -461,27 +462,30 @@ exports[`Dropdown matches snapshot for tab hidden (open) 1`] = `
>
show more
</button>
<ol
<menu
className="c4"
>
<li>
<a
href=""
<button
onClick={[Function]}
onMouseEnter={[Function]}
tabIndex={0}
type="button"
>
Delete
</a>
</button>
</li>
<li>
<a
href="/wooo"
onClick={[Function]}
onMouseEnter={[Function]}
role="button"
tabIndex={0}
>
Edit
</a>
</li>
</ol>
</menu>
</div>
`;
Loading
Loading