fix(all): fix design/code review feedbacks

chanzuckerberg · Apr 27, 2024 · 053fbf9 · 053fbf9
1 parent 5ab795f
commit 053fbf9
Show file tree

Hide file tree

Showing 34 changed files with 185 additions and 153 deletions.
diff --git a/packages/components/src/common/storybook/customSvgIcon.tsx b/packages/components/src/common/storybook/customSvgIcon.tsx
@@ -1,16 +1,13 @@
 import { SvgIcon, SvgIconProps } from "@mui/material";
-import { filterProps } from "../utils";
 
 /**
  * CustomSvgIcon is a component that extends the SvgIcon component from the Material-UI library.
  * It allows easy usage of custom SVG icons with in the storybook demos.
  */
-function CustomSvgIcon(props: SvgIconProps) {
-  const ExcludeProps = ["sdsSize"];
-  const finalProps = filterProps(props, ExcludeProps);
 
+function CustomSvgIcon(props: SvgIconProps) {
   return (
-    <SvgIcon color="primary" viewBox="2 2 20 20" {...finalProps}>
+    <SvgIcon color="primary" viewBox="2 2 20 20" {...props}>
       <path d="M7 19c-1.1 0-2 .9-2 2h14c0-1.1-.9-2-2-2h-4v-2h3c1.1 0 2-.9 2-2h-8c-1.66 0-3-1.34-3-3 0-1.09.59-2.04 1.46-2.56C8.17 9.03 8 8.54 8 8c0-.21.04-.42.09-.62C6.28 8.13 5 9.92 5 12c0 2.76 2.24 5 5 5v2z"></path>
       <path d="M10.56 5.51C11.91 5.54 13 6.64 13 8c0 .75-.33 1.41-.85 1.87l.59 1.62.94-.34.34.94 1.88-.68-.34-.94.94-.34-2.74-7.53-.94.34-.34-.94-1.88.68.34.94-.94.35z"></path>
       <circle cx="10.5" cy="8" r="1.5"></circle>

diff --git a/packages/components/src/common/utils.ts b/packages/components/src/common/utils.ts
@@ -6,14 +6,19 @@ export const toKebabCase = (str: string) =>
 export const EMPTY_OBJECT = {};
 
 // eslint-disable-next-line @typescript-eslint/no-explicit-any
-type PropsObject = Record<string, any>;
+type Props = { [key: string]: any };
 
-export const filterProps = <T extends PropsObject>(
-  obj: T,
-  excludedProps: string[]
-): T => {
-  const filteredEntries = Object.entries(obj).filter(
-    ([key]) => !excludedProps.includes(key)
-  );
-  return Object.fromEntries(filteredEntries) as T;
-};
+export function filterProps<T extends Props, K extends keyof T>(
+  props: T,
+  excludeProps: K[]
+): Partial<T> {
+  const result: Partial<T> = {};
+
+  for (const key of Object.keys(props) as (keyof T)[]) {
+    if (!excludeProps.includes(key as K)) {
+      result[key] = props[key];
+    }
+  }
+
+  return result;
+}
diff --git a/packages/components/src/core/Autocomplete/components/AutocompleteBase/index.tsx b/packages/components/src/core/Autocomplete/components/AutocompleteBase/index.tsx
@@ -13,7 +13,7 @@ import {
 import { useTheme } from "@mui/material/styles";
 import React, { ReactNode, SyntheticEvent, useCallback, useState } from "react";
 import { EMPTY_OBJECT, noop } from "src/common/utils";
-import ButtonIcon from "src/core/ButtonIcon";
+import Button from "src/core/Button";
 import { IconProps } from "src/core/Icon";
 import { InputSearchProps } from "src/core/InputSearch";
 import { StyledInputAdornment } from "src/core/InputSearch/style";
@@ -214,12 +214,13 @@ const AutocompleteBase = <
             endAdornment: (
               <StyledInputAdornment position="end">
                 {inputValue && (
-                  <ButtonIcon
+                  <Button
                     aria-label="clear-button"
                     className="input-search-clear-icon"
                     onClick={clearInput}
                     sdsType="tertiary"
                     sdsSize="small"
+                    sdsStyle="icon"
                     sdsIconProps={{
                       sdsType: "iconButton",
                     }}
@@ -231,11 +232,12 @@ const AutocompleteBase = <
             inputProps: params.inputProps,
             startAdornment: (
               <StyledInputAdornment position="start">
-                <ButtonIcon
+                <Button
                   aria-label="search-button"
                   onClick={clearInput}
                   sdsType="tertiary"
                   sdsSize="small"
+                  sdsStyle="icon"
                   sdsIconProps={{
                     sdsType: "interactive",
                   }}

diff --git a/packages/components/src/core/Autocomplete/components/AutocompleteMultiColumn/index.tsx b/packages/components/src/core/Autocomplete/components/AutocompleteMultiColumn/index.tsx
@@ -14,7 +14,7 @@ import {
   AutocompleteMultiColumnOption,
   AutocompleteMultiColumnValue,
 } from "src/core/Autocomplete";
-import ButtonIcon from "src/core/ButtonIcon";
+import Button from "src/core/Button";
 import { InputSearchProps } from "src/core/InputSearch";
 import { StyledInputAdornment } from "src/core/InputSearch/style";
 import { SDSTheme } from "src/core/styles";
@@ -199,11 +199,12 @@ const AutocompleteMultiColumn = <
                  * InputSearch's endAdornment, we must also include the clear IconButton here.
                  */}
                 {inputValue && (
-                  <ButtonIcon
+                  <Button
                     aria-label="clear-button"
                     className="input-search-clear-icon"
                     sdsType="tertiary"
                     sdsSize="small"
+                    sdsStyle="icon"
                     sdsIconProps={{
                       sdsType: "iconButton",
                     }}
@@ -220,10 +221,11 @@ const AutocompleteMultiColumn = <
             inputMode: "text",
             startAdornment: (
               <StyledInputAdornment position="start">
-                <ButtonIcon
+                <Button
                   aria-label="search-button"
                   sdsType="tertiary"
                   sdsSize="small"
+                  sdsStyle="icon"
                   sdsIconProps={{
                     sdsType: "interactive",
                   }}

diff --git a/packages/components/src/core/Banner/index.tsx b/packages/components/src/core/Banner/index.tsx
@@ -5,7 +5,7 @@ import {
   Centered,
   IconWrapper,
   StyledBanner,
-  StyledButtonIcon,
+  StyledButton,
 } from "./style";
 
 export interface BannerProps extends BannerExtraProps {
@@ -72,11 +72,12 @@ const Banner = forwardRef(function Banner(
         {children}
       </Centered>
       {dismissible && (
-        <StyledButtonIcon
+        <StyledButton
           aria-label="Close"
           bannerType={sdsType}
           sdsType="tertiary"
           sdsSize="small"
+          sdsStyle="icon"
           onClick={handleClose}
           icon="XMark"
         />

diff --git a/packages/components/src/core/Banner/style.ts b/packages/components/src/core/Banner/style.ts
@@ -1,6 +1,5 @@
 import { styled } from "@mui/material/styles";
-import ButtonIcon from "src/core/ButtonIcon";
-import { ButtonIconExtraProps } from "src/core/ButtonIcon/style";
+import Button, { ButtonProps } from "src/core/Button";
 import {
   CommonThemeProps,
   fontBodyS,
@@ -15,6 +14,8 @@ export interface BannerExtraProps extends CommonThemeProps {
   sdsType: "primary" | "secondary" | "tertiary";
 }
 
+type ButtonType = ButtonProps & { bannerType: string } & BannerExtraProps;
+
 export const Centered = styled("div")`
   flex: 1 1 auto;
   display: flex;
@@ -23,7 +24,7 @@ export const Centered = styled("div")`
 `;
 
 export const IconWrapper = styled("div")`
-  ${(props: ButtonIconExtraProps) => {
+  ${(props: CommonThemeProps) => {
     const iconSizes = getIconSizes(props);
     const spaces = getSpaces(props);
 
@@ -34,16 +35,15 @@ export const IconWrapper = styled("div")`
   }}
 `;
 
-type ButtonIconType = ButtonIconExtraProps & { bannerType: string };
 const doNotForwardPropsButtonIcon = ["bannerType", "textChild"];
 
-export const StyledButtonIcon = styled(ButtonIcon, {
+export const StyledButton = styled(Button as React.ComponentType<ButtonType>, {
   shouldForwardProp: (prop: string) =>
     !doNotForwardPropsButtonIcon.includes(prop),
 })`
   flex: 0 0 auto;
 
-  ${(props: ButtonIconType) => {
+  ${(props: ButtonType) => {
     const { bannerType } = props;
 
     const colors = getColors(props);

diff --git a/packages/components/src/core/Button/__storybook__/constants.tsx b/packages/components/src/core/Button/__storybook__/constants.tsx
@@ -2,7 +2,6 @@ import { action } from "@storybook/addon-actions";
 import CustomSdsIcon from "src/common/storybook/customSdsIcon";
 import CustomSvgIcon from "src/common/storybook/customSvgIcon";
 import { SDSSizes, SDSTypes } from "src/core/ButtonIcon/__storybook__/types";
-import Icon from "src/core/Icon";
 
 export const BUTTON_EXCLUDED_CONTROLS = [
   "endIcon",
@@ -61,10 +60,7 @@ export const BUTTON_ACTIONS = {
   onClick: action("onClick"),
 };
 
-export const SCREENSHOT_BUTTON_ICON_OPTIONS = [
-  undefined,
-  <Icon key="download" sdsSize="l" sdsIcon="Download" sdsType="button" />,
-];
+export const SCREENSHOT_BUTTON_ICON_OPTIONS = [undefined, "Download"];
 
 export const SCREENSHOT_BUTTON_SDS_STYLES = ["rounded", "square", "minimal"];
 

diff --git a/packages/components/src/core/Button/__storybook__/stories/screenshot.tsx b/packages/components/src/core/Button/__storybook__/stories/screenshot.tsx
@@ -17,7 +17,7 @@ import {
   SDSStyles,
   SDSTypes,
 } from "src/core/ButtonIcon/__storybook__/types";
-import { IconNameToSizes } from "src/core/Icon";
+import Icon, { IconNameToSizes } from "src/core/Icon";
 
 export const ScreenshotTestDemo = (props: Args): JSX.Element => {
   const topLevel: React.CSSProperties = {
@@ -126,26 +126,44 @@ export const ScreenshotTestDemo = (props: Args): JSX.Element => {
         <h4 style={secondLabel}>
           Type: <b>{type}</b>
         </h4>
-        {/* Minimal Secondary doesn't have icon button option */}
-        {sdsStyle === "minimal" && type === "secondary" ? (
-          <ButtonIconOption
-            sdsStyle={sdsStyle}
-            type={type}
-            icon={SCREENSHOT_BUTTON_ICON_OPTIONS[0]}
-            key={String(SCREENSHOT_BUTTON_ICON_OPTIONS[0])}
-          />
-        ) : (
-          SCREENSHOT_BUTTON_ICON_OPTIONS.map((icon) => {
-            return (
+        {SCREENSHOT_BUTTON_ICON_OPTIONS.map((icon) => {
+          return icon !== undefined ? (
+            sdsStyle === "minimal" ? (
               <ButtonIconOption
                 sdsStyle={sdsStyle}
                 type={type}
-                icon={icon}
+                icon={
+                  <Icon
+                    sdsIcon={icon as keyof IconNameToSizes}
+                    sdsType="button"
+                    sdsSize="s"
+                  />
+                }
+                key={String(icon)}
+              />
+            ) : (
+              <ButtonIconOption
+                sdsStyle={sdsStyle}
+                type={type}
+                icon={
+                  <Icon
+                    sdsIcon={icon as keyof IconNameToSizes}
+                    sdsType="button"
+                    sdsSize="l"
+                  />
+                }
                 key={String(icon)}
               />
-            );
-          })
-        )}
+            )
+          ) : (
+            <ButtonIconOption
+              sdsStyle={sdsStyle}
+              type={type}
+              icon={undefined}
+              key={String(icon)}
+            />
+          );
+        })}
       </div>
     );
   }
@@ -158,7 +176,7 @@ export const ScreenshotTestDemo = (props: Args): JSX.Element => {
   }: {
     sdsStyle: (typeof SCREENSHOT_BUTTON_SDS_STYLES)[number];
     type: (typeof SCREENSHOT_BUTTON_SDS_TYPES)[number];
-    icon: (typeof SCREENSHOT_BUTTON_ICON_OPTIONS)[number];
+    icon?: keyof IconNameToSizes | React.ReactElement<CustomSVGProps>;
   }) {
     return (
       <div style={displayContents}>
@@ -189,7 +207,7 @@ export const ScreenshotTestDemo = (props: Args): JSX.Element => {
   }: {
     sdsStyle: (typeof SCREENSHOT_BUTTON_SDS_STYLES)[number];
     type: (typeof SCREENSHOT_BUTTON_SDS_TYPES)[number];
-    icon: (typeof SCREENSHOT_BUTTON_ICON_OPTIONS)[number];
+    icon?: keyof IconNameToSizes | React.ReactElement<CustomSVGProps>;
     disabled: (typeof BUTTON_DISABLED_OPTIONS)[number];
   }) {
     return (

diff --git a/packages/components/src/core/Button/__tests__/__snapshots__/index.test.tsx.snap b/packages/components/src/core/Button/__tests__/__snapshots__/index.test.tsx.snap
@@ -2,7 +2,7 @@
 
 exports[`<button /> Default story renders snapshot 1`] = `
 <button
-  class="MuiButtonBase-root MuiButton-root MuiButton-contained MuiButton-containedPrimary MuiButton-sizeMedium MuiButton-containedSizeMedium MuiButton-root MuiButton-contained MuiButton-containedPrimary MuiButton-sizeMedium MuiButton-containedSizeMedium css-1tsf5k-MuiButtonBase-root-MuiButton-root"
+  class="MuiButtonBase-root MuiButton-root MuiButton-contained MuiButton-containedPrimary MuiButton-sizeMedium MuiButton-containedSizeMedium MuiButton-root MuiButton-contained MuiButton-containedPrimary MuiButton-sizeMedium MuiButton-containedSizeMedium css-n7hga1-MuiButtonBase-root-MuiButton-root"
   tabindex="0"
   text="Label"
   type="button"

diff --git a/packages/components/src/core/Button/index.tsx b/packages/components/src/core/Button/index.tsx
@@ -68,7 +68,9 @@ const Button = React.forwardRef(
     // isAllCaps is only used for the Minimal Button.  It defaults to true.
     const isAllCaps =
       typeof props?.isAllCaps === "boolean" ? props?.isAllCaps : true;
-    const propsWithDefault = { ...props, isAllCaps };
+
+    type PropsWithDefaultsType = ButtonProps & { isAllCaps: boolean };
+    const propsWithDefault: PropsWithDefaultsType = { ...props, isAllCaps };
 
     switch (true) {
       case sdsStyle === "rounded" && sdsType === "primary":
@@ -129,7 +131,7 @@ const Button = React.forwardRef(
         if (icon !== undefined) {
           // (masoudmanson): We need to remove the props that are not supported by
           // the ButtonIcon component.
-          const excludedProps = [
+          const excludedProps: (keyof PropsWithDefaultsType)[] = [
             "startIcon",
             "endIcon",
             "sdsStyle",