Merge pull request #5665 from Sage/FE-5567-padding-menu-item-pop-cont…

…ainer feat(icon-button): add padding interface to component FE-5567
Sage · Dec 14, 2022 · 33a9e57 · 33a9e57
2 parents cd29b36 + 2bce6b7
commit 33a9e57
Show file tree

Hide file tree

Showing 10 changed files with 124 additions and 32 deletions.
diff --git a/src/components/batch-selection/__snapshots__/batch-selection.spec.js.snap b/src/components/batch-selection/__snapshots__/batch-selection.spec.js.snap
@@ -38,7 +38,10 @@ exports[`BatchSelection component should render correctly 1`] = `
 .c3 {
   background: transparent;
   border: none;
-  padding: 0;
+}
+
+.c3.c3 {
+  padding: var(--spacing000);
 }
 
 .c3:focus {

diff --git a/src/components/icon-button/icon-button.component.tsx b/src/components/icon-button/icon-button.component.tsx
@@ -1,13 +1,12 @@
 import React, { useState, useCallback } from "react";
-import { MarginProps } from "styled-system";
+import { SpaceProps } from "styled-system";
 
 import Events from "../../__internal__/utils/helpers/events";
 import StyledIconButton from "./icon-button.style";
 import { IconProps } from "../icon";
-import { filterStyledSystemMarginProps } from "../../style/utils";
 import { TooltipProvider } from "../../__internal__/tooltip-provider";
 
-export interface IconButtonProps extends MarginProps {
+export interface IconButtonProps extends SpaceProps {
   /** Prop to specify the aria-label of the icon-button component */
   "aria-label"?: string;
   /** Icon meant to be rendered, should be an Icon component */
@@ -42,7 +41,6 @@ const IconButton = React.forwardRef<HTMLButtonElement, IconButtonProps>(
     ref
   ) => {
     const [internalRef, setInternalRef] = useState<HTMLButtonElement>();
-    const marginProps = filterStyledSystemMarginProps(rest);
 
     const handleKeyDown = (e: React.KeyboardEvent<HTMLButtonElement>) => {
       if (Events.isEnterKey(e) || Events.isSpaceKey(e)) {
@@ -69,13 +67,13 @@ const IconButton = React.forwardRef<HTMLButtonElement, IconButtonProps>(
 
     return (
       <StyledIconButton
+        p={0}
         {...rest}
         aria-label={ariaLabel}
         onKeyDown={handleKeyDown}
         onClick={handleOnClick}
         ref={setRefs}
         disabled={disabled}
-        {...marginProps}
       >
         <TooltipProvider
           disabled={disabled}

diff --git a/src/components/icon-button/icon-button.spec.tsx b/src/components/icon-button/icon-button.spec.tsx
@@ -6,7 +6,7 @@ import IconButton from ".";
 import Message from "../message";
 import {
   assertStyleMatch,
-  testStyledSystemMargin,
+  testStyledSystemSpacing,
 } from "../../__spec_helper__/test-utils";
 import StyledIconButton from "./icon-button.style";
 import Icon from "../icon";
@@ -95,11 +95,16 @@ describe("IconButton component", () => {
     });
 
     describe("styled system", () => {
-      testStyledSystemMargin((props) => (
-        <IconButton onAction={() => {}} {...props}>
-          <Icon type="home" />
-        </IconButton>
-      ));
+      testStyledSystemSpacing(
+        (props) => (
+          <IconButton onAction={() => {}} {...props}>
+            <Icon type="home" />
+          </IconButton>
+        ),
+        { p: 0 },
+        undefined,
+        { modifier: "&&" }
+      );
     });
 
     describe("on baseTheme", () => {

diff --git a/src/components/icon-button/icon-button.stories.mdx b/src/components/icon-button/icon-button.stories.mdx
@@ -51,4 +51,4 @@ import IconButton from "carbon-react/lib/components/icon-button";
 
 ### Icon Button
 
-<StyledSystemProps of={IconButton} noHeader margin />
+<StyledSystemProps of={IconButton} noHeader spacing />
diff --git a/src/components/icon-button/icon-button.style.ts b/src/components/icon-button/icon-button.style.ts
@@ -1,15 +1,16 @@
 import styled, { css } from "styled-components";
-import { margin } from "styled-system";
+import { space } from "styled-system";
 
 import StyledIcon from "../icon/icon.style";
 import { baseTheme } from "../../style/themes";
 
 const StyledIconButton = styled.button.attrs({ type: "button" })`
   ${({ disabled }: { disabled?: boolean }) => css`
-    ${margin}
+    && {
+      ${space}
+    }
     background: transparent;
     border: none;
-    padding: 0;
 
     &:focus {
       background-color: transparent;

diff --git a/src/components/menu/menu-item/menu-item.component.js b/src/components/menu/menu-item/menu-item.component.js
@@ -133,10 +133,9 @@ const MenuItem = ({
     maxWidth && typeof title === "string" ? title : "";
 
   const itemMaxWidth = !inFullscreenView ? maxWidth : undefined;
+  const asPassiveItem = !(onClick || href);
 
   if (submenu) {
-    const asPassiveItem = !(onClick || href);
-
     return (
       <StyledMenuItem
         data-component="menu-item"
@@ -187,6 +186,7 @@ const MenuItem = ({
         ariaLabel={ariaLabel}
         maxWidth={maxWidth}
         inFullscreenView={inFullscreenView}
+        asPassiveItem={asPassiveItem}
       >
         {clonedChildren}
       </StyledMenuItemWrapper>

diff --git a/src/components/menu/menu-item/menu-item.spec.js b/src/components/menu/menu-item/menu-item.spec.js
@@ -18,6 +18,8 @@ import StyledIcon from "../../icon/icon.style";
 import Icon from "../../icon/icon.component";
 import { StyledMenuItem } from "../menu.style";
 import menuConfigVariants from "../menu.config";
+import IconButton from "../../icon-button";
+import StyledIconButton from "../../icon-button/icon-button.style";
 
 const events = {
   enter: {
@@ -144,6 +146,61 @@ describe("MenuItem", () => {
         );
       });
 
+      it.each([
+        ["button", "onClick"],
+        ["a", "href"],
+      ])(
+        "should not set padding on the %s element if no %s passed",
+        (element) => {
+          wrapper = mount(
+            <MenuContext.Provider value={{ menuType }}>
+              <MenuItem>Item one</MenuItem>
+            </MenuContext.Provider>
+          );
+
+          expect(
+            wrapper.find(StyledMenuItemWrapper)
+          ).not.toHaveStyleRule("padding", "0 16px", { modifier: element });
+        }
+      );
+
+      it("applies the expected styling overrides when an IconButton is rendered as a child", () => {
+        wrapper = mount(
+          <MenuContext.Provider value={{ menuType }}>
+            <MenuItem>
+              <IconButton>
+                <Icon type="home" />
+              </IconButton>
+            </MenuItem>
+          </MenuContext.Provider>
+        );
+
+        assertStyleMatch(
+          {
+            display: "inline-flex",
+            marginRight: "0",
+          },
+          wrapper.find(StyledMenuItemWrapper),
+          { modifier: `${StyledIconButton} > span` }
+        );
+
+        assertStyleMatch(
+          {
+            outline: "none",
+          },
+          wrapper.find(StyledMenuItemWrapper),
+          { modifier: `${StyledIconButton}:focus` }
+        );
+
+        assertStyleMatch(
+          {
+            color: menuConfigVariants[menuType].color,
+          },
+          wrapper.find(StyledMenuItemWrapper),
+          { modifier: `${StyledIconButton}:focus [data-component="icon"]` }
+        );
+      });
+
       describe.each([
         ["button", "focus"],
         ["a", "focus"],

diff --git a/src/components/menu/menu-item/menu-item.style.js b/src/components/menu/menu-item/menu-item.style.js
@@ -1,6 +1,7 @@
 import styled, { css } from "styled-components";
 import { StyledLink } from "../../link/link.style";
-import IconStyle from "../../icon/icon.style";
+import StyledIcon from "../../icon/icon.style";
+import StyledIconButton from "../../icon-button/icon-button.style";
 import menuConfigVariants from "../menu.config";
 
 const StyledMenuItemWrapper = styled.a`
@@ -18,6 +19,7 @@ const StyledMenuItemWrapper = styled.a`
     inFullscreenView,
     overrideColor,
     as,
+    asPassiveItem,
   }) => css`
     display: inline-block;
     font-size: 14px;
@@ -85,11 +87,31 @@ const StyledMenuItemWrapper = styled.a`
       `
     }
 
-    a,
-    ${StyledLink} a,
-    button,
-    ${StyledLink} button {
-      padding: 0 16px;
+    ${
+      asPassiveItem
+        ? `
+        ${StyledIconButton} {
+          > span {
+            display: inline-flex;
+            margin-right: 0;
+          }
+
+          :focus {
+            outline: none;
+            [data-component="icon"] {
+              color: ${menuConfigVariants[menuType].color};
+            }
+          }
+        }
+      `
+        : `
+        a,
+        ${StyledLink} a,
+        button,
+        ${StyledLink} button {
+          padding: 0 16px;
+        }
+      `
     }
 
     button,
@@ -112,7 +134,7 @@ const StyledMenuItemWrapper = styled.a`
         color: ${menuConfigVariants[menuType].color};
       }
 
-      ${IconStyle} {
+      ${StyledIcon} {
         bottom: 1px;
       }
     }
@@ -226,7 +248,7 @@ const StyledMenuItemWrapper = styled.a`
         ${showDropdownArrow &&
         css`
           > a,
-          > button {
+          > button:not(${StyledIconButton}) {
             padding-right: 32px;
           }
 

diff --git a/src/components/navigation-bar/navigation-bar.stories.mdx b/src/components/navigation-bar/navigation-bar.stories.mdx
@@ -62,8 +62,8 @@ import NavigationBar from "carbon-react/lib/components/navigation-bar";
   <Story name="example with menu">
     <NavigationBar>
       <Menu>
-        <MenuItem>Menu Item One</MenuItem>
-        <MenuItem>Menu Item Two</MenuItem>
+        <MenuItem href="#">Menu Item One</MenuItem>
+        <MenuItem href="#">Menu Item Two</MenuItem>
       </Menu>
     </NavigationBar>
   </Story>
@@ -93,8 +93,8 @@ valid CSS string.
   <Story name="with custom spacing">
     <NavigationBar py={2} px={7}>
       <Menu>
-        <MenuItem>menu item one</MenuItem>
-        <MenuItem>menu item two</MenuItem>
+        <MenuItem href="#">menu item one</MenuItem>
+        <MenuItem href="#">menu item two</MenuItem>
       </Menu>
     </NavigationBar>
   </Story>

diff --git a/src/components/pod/__snapshots__/pod.spec.js.snap b/src/components/pod/__snapshots__/pod.spec.js.snap
@@ -84,7 +84,10 @@ exports[`ActionButtons StyledDeleteButton when isHovered prop is set should matc
 .c0 {
   background: transparent;
   border: none;
-  padding: 0;
+}
+
+.c0.c0 {
+  padding: var(--spacing000);
 }
 
 .c0:focus {
@@ -602,7 +605,10 @@ exports[`ActionButtons StyledUndoButton when isHovered prop is set should match
 .c0 {
   background: transparent;
   border: none;
-  padding: 0;
+}
+
+.c0.c0 {
+  padding: var(--spacing000);
 }
 
 .c0:focus {
Original file line number	Diff line number	Diff line change
Expand Up		@@ -51,4 +51,4 @@ import IconButton from "carbon-react/lib/components/icon-button";

		### Icon Button

		<StyledSystemProps of={IconButton} noHeader margin />
		<StyledSystemProps of={IconButton} noHeader spacing />