Skip to content

Commit

Permalink
refactor(MenuItem): separate MenuIcon logic from MenuItem (#2104)
Browse files Browse the repository at this point in the history
  • Loading branch information
YossiSaadi authored May 6, 2024
1 parent 7ea79e3 commit c2dec4a
Show file tree
Hide file tree
Showing 7 changed files with 168 additions and 61 deletions.
20 changes: 0 additions & 20 deletions packages/core/src/components/Menu/MenuItem/MenuItem.module.scss
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,6 @@
outline: none;
}

.iconWrapper {
margin-right: var(--spacing-small);
display: flex;
align-items: center;
justify-content: center;

.icon {
color: var(--icon-color);
}
}

.item.splitMenuItem {
padding: 0 0 0 var(--spacing-small);
}
Expand All @@ -35,11 +24,6 @@
color: var(--disabled-text-color);
}

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

.item:not(.disabled).focused {
background-color: var(--primary-background-hover-color);
}
Expand All @@ -57,10 +41,6 @@
border-radius: var(--border-radius-small);
}

.item:not(.disabled).selected .iconWrapper .icon {
color: var(--primary-color);
}

.title {
white-space: nowrap;
overflow: hidden;
Expand Down
51 changes: 12 additions & 39 deletions packages/core/src/components/Menu/MenuItem/MenuItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import React, {
useRef
} from "react";
import cx from "classnames";
import { isFunction } from "lodash-es";
import { ComponentDefaultTestId, getTestId } from "../../../tests/test-ids-utils";
import { DialogPosition } from "../../../constants/positions";
import Text from "../../Text/Text";
Expand All @@ -31,6 +30,7 @@ import { CloseMenuOption, MenuChild } from "../Menu/MenuConstants";
import Label from "../../Label/Label";
import styles from "./MenuItem.module.scss";
import useIsMouseEnter from "../../../hooks/useIsMouseEnter";
import MenuItemIcon from "./components/MenuItemIcon/MenuItemIcon";
import MenuItemSubMenuIcon from "./components/MenuItemSubMenuIcon/MenuItemSubMenuIcon";

export interface MenuItemProps extends VibeComponentProps {
Expand Down Expand Up @@ -237,43 +237,6 @@ const MenuItem: VibeComponent<MenuItemProps | MenuItemTitleComponentProps> & {
// if "title" is a component ariaLabel is mandatory
const iconLabel = ariaLabel ?? (title as string);

const [iconWrapperStyle, iconStyle] = useMemo(() => {
return iconBackgroundColor
? [
{
backgroundColor: iconBackgroundColor,
borderRadius: "4px",
opacity: disabled ? 0.4 : 1
},
{ color: "var(--text-color-on-primary)" }
]
: [];
}, [iconBackgroundColor, disabled]);

const renderMenuItemIconIfNeeded = () => {
if (!icon) return null;

let finalIconType = iconType;
if (!finalIconType) {
finalIconType = isFunction(icon) ? Icon.type.SVG : Icon.type.ICON_FONT;
}

return (
<div className={cx(styles.iconWrapper, iconWrapperClassName)} style={iconWrapperStyle}>
<Icon
iconType={finalIconType}
clickable={false}
icon={icon}
iconLabel={iconLabel}
className={styles.icon}
ignoreFocusStyle
style={iconStyle}
iconSize={18}
/>
</div>
);
};

const a11yProps = useMemo(() => {
if (!children) return {};
return {
Expand Down Expand Up @@ -314,7 +277,17 @@ const MenuItem: VibeComponent<MenuItemProps | MenuItemTitleComponentProps> & {
onMouseEnter={onMouseEnter}
tabIndex={TAB_INDEX_FOCUS_WITH_JS_ONLY}
>
{renderMenuItemIconIfNeeded()}
{Boolean(icon) && (
<MenuItemIcon
icon={icon}
type={iconType}
label={iconLabel}
disabled={disabled}
selected={selected}
backgroundColor={iconBackgroundColor}
wrapperClassName={iconWrapperClassName}
/>
)}
<Tooltip
content={shouldShowTooltip ? finalTooltipContent : null}
position={tooltipPosition}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,12 @@ exports[`Snapshots renders correctly with title and SVG icon 1`] = `
tabIndex={-1}
>
<div
className="iconWrapper"
className="container directionRow justifyCenter alignCenter iconWrapper"
style={
Object {
"gap": "0px",
}
}
>
<svg
aria-hidden={true}
Expand Down Expand Up @@ -260,7 +265,12 @@ exports[`Snapshots renders correctly with title and font icon 1`] = `
tabIndex={-1}
>
<div
className="iconWrapper"
className="container directionRow justifyCenter alignCenter iconWrapper"
style={
Object {
"gap": "0px",
}
}
>
<span
aria-hidden={true}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
.iconWrapper {
margin-right: var(--spacing-small);

&.withBackgroundColor {
border-radius: var(--spacing-xs);

&.disabled {
opacity: 0.4;
}

.icon {
color: var(--text-color-on-primary);
}
}

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

.icon {
color: var(--icon-color);

&.selected {
color: var(--primary-color);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import React from "react";
import Icon from "../../../../Icon/Icon";
import Flex from "../../../../Flex/Flex";
import cx from "classnames";
import styles from "./MenuItemIcon.module.scss";
import { MenuItemIconProps } from "./MenuItemIcon.types";

const MenuItemIcon = ({
icon,
type,
label,
disabled,
selected,
backgroundColor,
wrapperClassName
}: MenuItemIconProps) => (
<Flex
justify={Flex.justify.CENTER}
className={cx(
styles.iconWrapper,
{
[styles.disabled]: disabled,
[styles.withBackgroundColor]: !!backgroundColor
},
wrapperClassName
)}
style={{ ...(backgroundColor && { backgroundColor }) }}
>
<Icon
iconType={type || (typeof icon === "function" ? Icon.type.SVG : Icon.type.ICON_FONT)}
clickable={false}
icon={icon}
iconLabel={label}
className={cx(styles.icon, { [styles.selected]: !disabled && selected })}
ignoreFocusStyle
iconSize={18}
/>
</Flex>
);

export default MenuItemIcon;
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { SubIcon } from "../../../../../types";
import { IconType } from "../../../../Icon/IconConstants";

export interface MenuItemIconProps {
/**
* Icon to be displayed. Can be a string or an icon component.
*/
icon?: SubIcon;
/**
* Icon type to be used.
*/
type?: IconType;
/**
* Label for the icon, used for accessibility.
*/
label?: string;
/**
* Indicates whether the icon is disabled. Disabled icons appear faded and do not respond to user interactions.
*/
disabled?: boolean;
/**
* Indicates whether the icon is selected. Selected icons have a different visual style.
*/
selected?: boolean;
/**
* Background color for the icon wrapper.
*/
backgroundColor?: string;
/**
* Additional class name for the icon wrapper.
*/
wrapperClassName?: string;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
import { render, screen } from "@testing-library/react";
import "@testing-library/jest-dom";
import MenuItemIcon from "../MenuItemIcon";
import { MenuItemIconProps } from "../MenuItemIcon.types";

function renderMenuItemIcon(props: Partial<MenuItemIconProps> = {}) {
return render(<MenuItemIcon icon="test-icon" label="Test Icon" {...props} />);
}

describe("MenuItemIcon", () => {
it("should render the icon correctly", () => {
const { getByLabelText } = renderMenuItemIcon();
expect(getByLabelText("Test Icon")).toBeInTheDocument();
});

it("should apply the disabled style if the disabled prop is true", () => {
const { getByLabelText } = renderMenuItemIcon({ disabled: true });
const iconWrapper = getByLabelText("Test Icon").parentNode;
expect(iconWrapper).toHaveClass("disabled");
});

it("should apply the selected style if the selected prop is true and not disabled", () => {
const { getByLabelText } = renderMenuItemIcon({ selected: true });
screen.logTestingPlaygroundURL();
expect(getByLabelText("Test Icon")).toHaveClass("selected");
});

it("should not apply the selected style if the icon is disabled", () => {
const { getByLabelText } = renderMenuItemIcon({
selected: true,
disabled: true
});
expect(getByLabelText("Test Icon")).not.toHaveClass("selected");
});

it("should set the background color if backgroundColor prop is provided", () => {
const { getByLabelText } = renderMenuItemIcon({ backgroundColor: "red" });
const iconWrapper = getByLabelText("Test Icon").parentNode;
expect(iconWrapper).toHaveClass("withBackgroundColor");
expect(iconWrapper).toHaveStyle("background-color: red");
});
});

0 comments on commit c2dec4a

Please sign in to comment.