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

refactor(MenuItem): separate SubMenuIcon logic from MenuItem #2100

Merged
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
37 changes: 1 addition & 36 deletions packages/core/src/components/Menu/MenuItem/MenuItem.module.scss
Original file line number Diff line number Diff line change
Expand Up @@ -17,45 +17,11 @@

.iconWrapper {
margin-right: var(--spacing-small);
}

.subMenuIconWrapper {
margin-left: var(--spacing-xs);
}

.subMenuIconWrapper,
.iconWrapper {
display: flex;
align-items: center;
justify-content: center;
}

.subMenuIconWrapper .subMenuIcon,
.subMenuIconWrapper .icon,
.iconWrapper .subMenuIcon,
.iconWrapper .icon {
color: var(--icon-color);
}

.subMenuIconWrapper .divider {
height: 19px;
}

.subMenuIconWrapper .splitMenuItemIconButton {
width: 34px;
height: 30px;

// TODO Temp override for Tal changes - remove once token is changed
--primary-background-hover-color: rgba(103, 104, 121, 0.1);
// Override for IconButton active color
--primary-selected-color: rgba(103, 104, 121, 0.1);
}

.iconButton {
width: 18px;
height: 18px;

&:not(.disabled) {
.icon {
color: var(--icon-color);
}
}
Expand All @@ -69,7 +35,6 @@
color: var(--disabled-text-color);
}

.item.disabled .subMenuIcon,
.item.disabled .icon {
cursor: not-allowed;
color: var(--disabled-text-color);
Expand Down
46 changes: 10 additions & 36 deletions packages/core/src/components/Menu/MenuItem/MenuItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,8 @@ import { TAB_INDEX_FOCUS_WITH_JS_ONLY, TooltipPosition } from "./MenuItemConstan
import { CloseMenuOption, MenuChild } from "../Menu/MenuConstants";
import Label from "../../Label/Label";
import styles from "./MenuItem.module.scss";
import { DropdownChevronRight } from "../../Icon/Icons";
import IconButton from "../../IconButton/IconButton";
import Divider from "../../Divider/Divider";
import { DirectionType } from "../../Divider/DividerConstants";
import useIsMouseEnter from "../../../hooks/useIsMouseEnter";
import MenuItemSubMenuIcon from "./components/MenuItemSubMenuIcon/MenuItemSubMenuIcon";

export interface MenuItemProps extends VibeComponentProps {
title?: string;
Expand Down Expand Up @@ -239,37 +236,6 @@ const MenuItem: VibeComponent<MenuItemProps | MenuItemTitleComponentProps> & {

// if "title" is a component ariaLabel is mandatory
const iconLabel = ariaLabel ?? (title as string);
const renderSubMenuIconIfNeeded = () => {
if (!hasChildren) return null;

return splitMenuItem ? (
<div className={styles.subMenuIconWrapper}>
<Divider direction={DirectionType.VERTICAL} className={styles.divider} />
<IconButton
icon={DropdownChevronRight}
className={styles.splitMenuItemIconButton}
kind={IconButton.kinds.TERTIARY}
size={null} // Customizing size via className
iconClassName={cx(styles.iconButton, { [styles.disabled]: disabled })}
tabIndex={-1}
ref={iconButtonElementRef}
active={shouldShowSubMenu}
disabled={disabled}
/>
</div>
) : (
<div className={styles.subMenuIconWrapper}>
<Icon
clickable={false}
icon={DropdownChevronRight}
iconLabel={iconLabel}
className={styles.subMenuIcon}
ignoreFocusStyle
iconSize={18}
/>
</div>
);
};

const [iconWrapperStyle, iconStyle] = useMemo(() => {
return iconBackgroundColor
Expand Down Expand Up @@ -364,7 +330,15 @@ const MenuItem: VibeComponent<MenuItemProps | MenuItemTitleComponentProps> & {
</div>
</Tooltip>
{label && <Label kind={Label.kinds.LINE} text={label} />}
{renderSubMenuIconIfNeeded()}
{hasChildren && (
<MenuItemSubMenuIcon
YossiSaadi marked this conversation as resolved.
Show resolved Hide resolved
ref={iconButtonElementRef}
isSplit={splitMenuItem}
label={iconLabel}
active={shouldShowSubMenu}
disabled={disabled}
/>
)}
<div
style={{ ...popoverStyles.popper, visibility: shouldShowSubMenu ? "visible" : "hidden" }}
// eslint-disable-next-line react/jsx-props-no-spreading
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Canvas, Meta } from "@storybook/blocks";
import { Meta } from "@storybook/blocks";
import { ComponentRuleWithLabelDo, ComponentRuleWithLabelDont } from "./MenuItem.stories.helpers";
import * as MenuItemStories from "./MenuItem.stories";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ import { Activity } from "../../../Icon/Icons";
import Icon from "../../../Icon/Icon";
import Tooltip from "../../../Tooltip/Tooltip";
import { createStoryMetaSettingsDecorator } from "../../../../storybook/functions/createStoryMetaSettingsDecorator";
import { Meta, StoryObj } from "@storybook/react";

type Story = StoryObj<MenuItemProps>;

const metaSettings = createStoryMetaSettingsDecorator({
component: MenuItem,
Expand All @@ -18,60 +21,68 @@ export default {
component: MenuItem,
argTypes: metaSettings.argTypes,
decorators: metaSettings.decorators
};
} satisfies Meta<typeof MenuItem>;
YossiSaadi marked this conversation as resolved.
Show resolved Hide resolved

const menuItemTemplate = (args: MenuItemProps) => (
<Menu>
<MenuItem {...args} />
</Menu>
);

export const Overview = {
export const Overview: Story = {
render: menuItemTemplate.bind({}),
name: "Overview",

args: {
title: "Menu item"
},
parameters: {
docs: {
liveEdit: {
isEnabled: false
}
}
}
};

export const States = {
export const States: Story = {
render: () => (
<Menu>
<MenuItem title="Regular menu item" />
<MenuItem title="Selected menu item" selected />
<MenuItem title="Disabled menu item" disabled />
</Menu>
),
name: "States"
)
};

export const Icons = {
export const Icons: Story = {
render: () => (
<Menu>
<MenuItem title="SVG icon" icon={Activity} />
<MenuItem title="Font icon" icon="fa fa-star" iconType={MenuItem.iconType.ICON_FONT} />
</Menu>
),
name: "Icons"
parameters: {
docs: {
liveEdit: {
scope: { Activity }
Copy link
Member

Choose a reason for hiding this comment

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

What it this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

<MenuItem title="SVG icon" icon={Activity} />

I need to add this icon to the scope
otherwise it will only know what is VibeIcons.Activity, but the story says Activity

}
}
}
};

export const Label = {
export const Label: Story = {
render: () => (
<Menu>
<MenuItem title="Menu item" label="New" />
</Menu>
),
name: "Label",

parameters: {
chromatic: {
pauseAnimationAtEnd: true
}
}
};

export const SubMenu = {
export const SubMenu: Story = {
render: () => (
<Menu>
<MenuItem title="Opens on item hover">
Expand All @@ -81,7 +92,7 @@ export const SubMenu = {
<MenuItem title="Sub menu item 3" onClick={() => alert("clicked on sub menu item 3")} />
</Menu>
</MenuItem>
<MenuItem title="Opens on icon hover" splitMenuItem={true} onClick={() => alert("clicked on menu item")}>
<MenuItem title="Opens on icon hover" splitMenuItem onClick={() => alert("clicked on menu item")}>
<Menu tabIndex={0} id="sub-menu">
<MenuItem title="Sub menu item 1" onClick={() => alert("clicked on sub menu item 1")} />
<MenuItem title="Sub menu item 2" onClick={() => alert("clicked on sub menu item 2")} />
Expand All @@ -93,7 +104,7 @@ export const SubMenu = {
name: "Sub menu"
};

export const Overflow = {
export const Overflow: Story = {
render: () => (
<Menu>
<MenuItem title="short text" />
Expand All @@ -106,11 +117,10 @@ export const Overflow = {
</Menu>
</MenuItem>
</Menu>
),
name: "Overflow"
)
};

export const TooltipStory = {
export const TooltipStory: Story = {
render: () => (
<Menu>
<MenuItem title="Menu item with tooltip" tooltipContent="I am tooltip" />
Expand All @@ -130,5 +140,12 @@ export const TooltipStory = {
/>
</Menu>
),
name: "Tooltip"
name: "Tooltip",
parameters: {
docs: {
liveEdit: {
scope: { Activity }
}
}
}
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
.subMenuIconWrapper {
margin-left: var(--spacing-xs);

.divider {
height: 19px;
}

.splitSubMenuIcon {
width: 18px;
height: 18px;

&.disabled {
cursor: not-allowed;
color: var(--disabled-text-color);
}
}

.subMenuIcon,
.splitSubMenuIcon {
color: var(--icon-color);
}

.splitMenuItemIconButton {
width: 34px;
height: 30px;

// TODO Temp override for Tal changes - remove once token is changed
--primary-background-hover-color: rgba(103, 104, 121, 0.1);
talkor marked this conversation as resolved.
Show resolved Hide resolved
// Override for IconButton active color
--primary-selected-color: rgba(103, 104, 121, 0.1);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import React, { forwardRef } from "react";
import cx from "classnames";
import Divider from "../../../../Divider/Divider";
import Icon from "../../../../Icon/Icon";
import Flex from "../../../../Flex/Flex";
import IconButton from "../../../../IconButton/IconButton";
import { DropdownChevronRight } from "../../../../Icon/Icons";
import styles from "./MenuItemSubMenuIcon.module.scss";
import { MenuItemSubMenuIconProps } from "./MenuItemSubMenuIcon.types";

const MenuItemSubMenuIcon = forwardRef((props: MenuItemSubMenuIconProps, ref: React.ForwardedRef<HTMLDivElement>) => (
<Flex justify={Flex.justify.CENTER} className={styles.subMenuIconWrapper}>
{props.isSplit === true ? (
<>
<Divider direction={Divider.directions.VERTICAL} className={styles.divider} />
<IconButton
icon={DropdownChevronRight}
className={styles.splitMenuItemIconButton}
kind={IconButton.kinds.TERTIARY}
size={null} // Customizing size via className
iconClassName={cx(styles.splitSubMenuIcon, { [styles.disabled]: props.disabled })}
tabIndex={-1}
ref={ref}
active={props.active}
disabled={props.disabled}
/>
</>
) : (
<Icon
clickable={false}
icon={DropdownChevronRight}
iconLabel={props.label}
className={styles.subMenuIcon}
ignoreFocusStyle
iconSize={18}
/>
)}
</Flex>
));

export default MenuItemSubMenuIcon;
YossiSaadi marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
export interface SplitMenuItemSubMenuIconProps {
/**
* Determines whether the submenu icon is split from the main menu item.
* When true, clicking the main menu item and the submenu icon trigger different actions.
*/
isSplit?: true;
/**
* Indicates if the split submenu is currently active.
*/
active?: boolean;
/**
* Whether the split submenu icon is disabled.
*/
disabled?: boolean;
}

export interface SimpleMenuItemSubMenuIconProps {
isSplit?: false;
/**
* Label for the submenu icon, used for accessibility.
*/
label?: string;
}

export type MenuItemSubMenuIconProps = SimpleMenuItemSubMenuIconProps | SplitMenuItemSubMenuIconProps;
Loading