fixed code review comments

NYPL · Nov 18, 2024 · 0aeb859 · 0aeb859
1 parent 2bd0417
commit 0aeb859
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 11 deletions.
diff --git a/src/components/SubNav/SubNav.stories.tsx b/src/components/SubNav/SubNav.stories.tsx
@@ -160,7 +160,7 @@ export const OutlinedItem: Story = {
             }
             screenreaderOnlyText="Education for babies and toddlers"
           >
-            Babies &amp; Toddlers
+            Babies &amp; toddlers
           </SubNavButton>
           <SubNavLink
             href="#link1"
@@ -294,9 +294,17 @@ export const UsingIconsMobile: Story = {
 };
 
 export const CustomColors: Story = {
-  render: () => (
+  args: {
+    className: undefined,
+    actionBackgroundColor: "brand.primary-05",
+    highlightColor: "brand.primary",
+    id: "subnav-colors",
+  },
+
+  render: (args: any) => (
     <>
       <SubNav
+        {...args}
         primaryActions={
           <>
             <SubNavButton
@@ -306,7 +314,7 @@ export const CustomColors: Story = {
               }
               screenreaderOnlyText="Education for babies and toddlers"
             >
-              Babies &amp; Toddlers
+              Babies &amp; toddlers
             </SubNavButton>
             <SubNavLink
               href="#link1"
@@ -445,7 +453,14 @@ export const HorizontalScrolling: Story = {
 };
 
 export const Placement: Story = {
-  render: () => (
+  args: {
+    className: undefined,
+    actionBackgroundColor: "section.research.primary-05",
+    highlightColor: "section.research.primary",
+    id: "subnav-placement",
+  },
+
+  render: (args: any) => (
     <>
       <Hero
         backgroundColor="section.research.primary"
@@ -460,6 +475,7 @@ export const Placement: Story = {
         heroType="tertiary"
       />
       <SubNav
+        {...args}
         primaryActions={
           <>
             <SubNavButton

diff --git a/src/components/SubNav/SubNav.tsx b/src/components/SubNav/SubNav.tsx
@@ -96,13 +96,30 @@ export interface SubNavProps {
   secondaryActions?: React.ReactNode;
 }
 
-export const SubNavButton: React.FC<React.PropsWithChildren<any>> = ({
+interface SubNavItemProps {
+  id: string;
+  children: React.ReactNode;
+  isOutlined?: boolean;
+  isSelected?: boolean;
+  screenreaderOnlyText?: string;
+}
+
+interface SubNavLinkProps extends SubNavItemProps {
+  href: string;
+}
+
+interface SubNavButtonProps extends SubNavItemProps {
+  onClick: (event: React.MouseEvent | React.KeyboardEvent) => void;
+}
+
+export const SubNavButton: React.FC<React.PropsWithChildren<SubNavButtonProps>> = ({
   id,
   children,
   isOutlined,
   isSelected,
   screenreaderOnlyText = "",
 }) => {
+
   const childrenStyles = useMultiStyleConfig("SubNavChildren", {
     isOutlined: isOutlined,
   });
@@ -123,7 +140,7 @@ export const SubNavButton: React.FC<React.PropsWithChildren<any>> = ({
   );
 };
 
-export const SubNavLink: React.FC<React.PropsWithChildren<any>> = ({
+export const SubNavLink: React.FC<React.PropsWithChildren<SubNavLinkProps>> = ({
   id,
   children,
   isOutlined,
@@ -213,12 +230,11 @@ export const SubNav: ChakraComponent<
       });
 
       return (
-        <Box as="nav" aria-label="Sub-navigation menu" __css={styles.base}>
+        <Box as="nav" aria-label="Sub-navigation menu" className={className} __css={styles.base}>
           <Box __css={styles.container}>
             <Flex
               alignItems="center"
-              className={className}
-              gap="1rem"
+              gap="s"
               justify="space-between"
             >
               <div style={styles.primaryList}>

diff --git a/src/theme/components/subnav.ts b/src/theme/components/subnav.ts
@@ -149,10 +149,10 @@ const SubNav = subNavDefineMultiStyleConfig({
             ...primaryActionsStyles,
           },
           a: {
-            color: `${highlightOrDefaultColor} !important`,
+            color: `${highlightOrDefaultColor}`,
             ...primaryActionsStyles,
             svg: {
-              fill: `${highlightOrDefaultColor} !important`,
+              fill: `${highlightOrDefaultColor}`,
               margin: { base: "0", md: null },
               _dark: {
                 fill: "ui.white !important",