Skip to content

feat(menu): add optional danger state for menu items #8131

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.

Sign up for GitHub

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

Merged
tlabaj merged 3 commits into from
Oct 21, 2022
+37 −0
Merged

feat(menu): add optional danger state for menu items #8131

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03e4a98
feat(menu): add optional danger state for menu items
gefgu Sep 30, 2022
fe9df92
docs(menu): add demo for danger state
gefgu Oct 6, 2022
8d30b55
chore: move example to match html
gefgu Oct 18, 2022
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
4 changes: 4 additions & 0 deletions packages/react-core/src/components/Menu/MenuItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ export interface MenuItemProps extends Omit<React.HTMLProps<HTMLLIElement>, 'onC
isSelected?: boolean;
/** Flag indicating the item is focused */
isFocused?: boolean;
/** Flag indicating the item is in danger state */
isDanger?: boolean;
/** @beta Flyout menu */
flyoutMenu?: React.ReactElement;
/** @beta Callback function when mouse leaves trigger */
Expand Down Expand Up @@ -91,6 +93,7 @@ const MenuItemBase: React.FunctionComponent<MenuItemProps> = ({
isExternalLink = false,
isSelected = null,
isFocused,
isDanger = false,
icon,
actions,
onShowFlyout,
Expand Down Expand Up @@ -295,6 +298,7 @@ const MenuItemBase: React.FunctionComponent<MenuItemProps> = ({
isLoadButton && styles.modifiers.load,
isLoading && styles.modifiers.loading,
isFocused && styles.modifiers.focus,
isDanger && styles.modifiers.danger,
Copy link
Copy Markdown
Contributor

@mcoker mcoker Oct 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically for these statuses, we include some sr-only text

patternfly-react/packages/react-core/src/components/Popover/Popover.tsx

Line 452 in 60f9853

alertSeverityScreenReaderText={alertSeverityScreenReaderText || `${alertSeverityVariant} alert:`}

We have the sr-only text in the core example. Should we add that? cc @thatblindgeye

Copy link
Copy Markdown
Contributor

@thatblindgeye thatblindgeye Oct 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This feels like an interesting scenario to me. I think it could really go either way, but it would bring into question whether the "danger"/"warning" variant of our Button component should have similar visually hidden text.

I don't believe I've seen visually hidden text like this used for "danger" menu items/buttons before, and it could possibly stray into being too verbose (especially the more visually hidden text there is on a page)... but at the same time, if we want to visually convey these menu items as "danger" items to users that are sighted, regardless if there's an additional step from a confirmation modal, it could make sense to convey that a menu item is a "danger" item to users of AT similarly.

Looking at Web-AIM's invisible content page in the "Examples" section, they have a note about:

Use these techniques judiciously! Keep in mind that many screen reader users have some vision—what they see and what they hear should typically be in harmony. In general, screen reader-only content should be reserved for information is apparent visually but not apparent to blind screen reader users.

Which this scenario could technically fit into that sort of criteria (a "danger" item is apparent visually via the text color, but not as such to users that are blind and navigating via SR).

It could possibly come down to whether the text of the "danger" menu item is enough to convey that it is a dangerous/destructive action (i.e. will these dangerous items always be as clear as "Delete"), or whether the additional context via color and/or visually hidden text is needed to help convey that dangerous/destruction nature (i.e. is the text color more to convey an item as dangerous/destructive, or more to just visually separate it from other menu items?).

I think I'd generally be okay with SR text being added, but it would probably lead to a broader convo regarding other components like Button that also have status/severity sort of visual styling.

className
)}
onMouseOver={onMouseOver}
Expand Down
6 changes: 5 additions & 1 deletion packages/react-core/src/components/Menu/examples/Menu.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ import AngleLeftIcon from '@patternfly/react-icons/dist/esm/icons/angle-left-ico

```ts file="MenuBasic.tsx"
```

### With icons

```ts file="MenuWithIcons.tsx"
Expand Down Expand Up @@ -119,3 +118,8 @@ To render an initially drilled in menu, the `menuDrilledIn`, `drilldownPath`, an

```ts file="MenuWithViewMore.tsx"
```

### Danger menu item
Copy link
Copy Markdown
Contributor

@tlabaj tlabaj Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move this example up so it is the second one on the page. Typically we try and keep our example order aligned with the HTML examples when possible.

Copy link
Copy Markdown
Contributor

@mcoker mcoker Oct 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tlabaj oh, I made a comment in the core PR to move it after the "view more" and "loading" examples, since those seem like similar variations, but I missed that it wasn't made before merging. WDYT? patternfly/patternfly#5102 (review)


```ts file="MenuDangerMenuItem.tsx"
```
28 changes: 28 additions & 0 deletions packages/react-core/src/components/Menu/examples/MenuDangerMenuItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import React from 'react';
import { Divider, Menu, MenuContent, MenuItem, MenuList } from '@patternfly/react-core';

export const MenuDangerMenuItem: React.FunctionComponent = () => {
const [activeItem, setActiveItem] = React.useState(0);

const onSelect = (_event: React.MouseEvent<Element, MouseEvent> | undefined, itemId: number | string | undefined) => {
const item = itemId as number;
// eslint-disable-next-line no-console
console.log(`clicked ${itemId}`);
Copy link
Copy Markdown
Contributor Author

@gefgu gefgu Oct 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made this log call for consistency with the other examples.

setActiveItem(item);
};

return (
<Menu activeItemId={activeItem} onSelect={onSelect}>
<MenuContent>
<MenuList>
<MenuItem itemId={0}>Action 1</MenuItem>
<MenuItem itemId={1}>Action 2</MenuItem>
<Divider component="li" />
<MenuItem itemId={2} isDanger>
Delete
</MenuItem>
</MenuList>
</MenuContent>
</Menu>
);
};