fix(component-header-footer): pass onClick handler to header buttons

Fixed an issue where the onClick function prop wasn't being properly passed down to buttons in the
header component, while all other props were correctly passing through.

Author: davidornelas11

packages/component-header-footer/src/header/components/Button/index.js

@@ -20,6 +20,7 @@ const Button = ({ href, color, text, classes, onClick, onFocus }) => {
       className={`button-${color} ${classes ?? ""}`}
       onClick={onClick}
       onFocus={onFocus}
+      {...(onClick && { role: "button" })}
     >
       {text}
     </ButtonWrapper>

packages/component-header-footer/src/header/components/HeaderMain/NavbarContainer/Buttons.test.js

@@ -0,0 +1,157 @@
+/* eslint-disable jest/no-mocks-import */
+// @ts-check
+import { render, fireEvent, cleanup } from "@testing-library/react";
+import React from "react";
+import { trackGAEvent } from "../../../../../../../shared";
+
+jest.mock("../../../../../../../shared", () => ({
+  idGenerator: jest.fn().mockImplementation(function* () {
+    yield "test-id";
+  }),
+  trackGAEvent: jest.fn(),
+}));
+
+import { NavbarContainer, BUTTON_ERROR_MESSAGE } from ".";
+import { AppContextProvider } from "../../../core/context/app-context";
+
+const stateWithButtons = {
+  navTree: [
+    {
+      href: "/",
+      text: "Home",
+      type: "icon-home",
+      class: "home",
+    },
+    {
+      text: "Link option 1",
+      href: "#",
+      items: [
+        [
+          {
+            href: "https://www.asu.edu",
+            text: "Sublink 1",
+          },
+          {
+            href: "https://www.asu.edu",
+            text: "Sublink 2",
+          },
+          {
+            href: "https://www.asu.edu",
+            text: "Sublink 3",
+          },
+          {
+            href: "https://www.asu.edu",
+            text: "Sublink 4",
+          },
+          {
+            href: "https://www.asu.edu",
+            text: "Sublink 5",
+          },
+          {
+            href: "https://www.asu.edu",
+            text: "Sublink 6",
+          },
+          {
+            href: "https://www.asu.edu",
+            text: "Sublink 7",
+          },
+          {
+            href: "https://www.asu.edu",
+            text: "Sublink 8",
+          },
+        ],
+      ],
+    },
+  ],
+  buttons: [
+    { text: "Login", href: "/login" },
+    { text: "Sign Up", onClick: jest.fn() },
+    { text: "Contact", onClick: jest.fn(), href: "#" }
+  ],
+  breakpoint: "lg"
+};
+
+const renderNavbarContainer = (props) => {
+  return render(
+    <AppContextProvider initialValue={props}>
+      <NavbarContainer />
+    </AppContextProvider>
+  );
+};
+
+describe("#NavbarContainer Button Tests", () => {
+  /** @type {import("@testing-library/react").RenderResult} */
+  let component;
+
+  beforeEach(() => {
+    jest.clearAllMocks();
+    component = renderNavbarContainer(stateWithButtons);
+  });
+
+  afterEach(cleanup);
+
+  it("should render all buttons from context", () => {
+    const buttonsContainer = component.getByTestId("buttons-container");
+    expect(buttonsContainer).toBeInTheDocument();
+
+    const buttons = component.getAllByRole("button");
+    expect(buttons.length).toBe(stateWithButtons.buttons.length);
+
+    stateWithButtons.buttons.forEach((buttonData, index) => {
+      expect(buttons[index]).toHaveTextContent(buttonData.text);
+    });
+  });
+
+  it("should call the onClick handler when a button is clicked", () => {
+    const buttons = component.getAllByRole("button");
+
+    fireEvent.click(buttons[1]);
+
+    expect(stateWithButtons.buttons[1].onClick).toHaveBeenCalledTimes(1);
+  });
+
+  it("should track GA event when any button is clicked", () => {
+    const buttons = component.getAllByRole("button");
+
+    buttons.forEach((button, index) => {
+      fireEvent.click(button);
+
+      expect(trackGAEvent).toHaveBeenCalledWith({
+        event: "link",
+        action: "click",
+        name: "onclick",
+        region: "navbar",
+        type: "internal link",
+        section: "main navbar",
+        text: stateWithButtons.buttons[index].text,
+      });
+    });
+
+    expect(trackGAEvent).toHaveBeenCalledTimes(buttons.length);
+  });
+
+  it("should log an error for buttons with both onClick and href", () => {
+    cleanup();
+
+    const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {});
+
+    renderNavbarContainer(stateWithButtons);
+
+    expect(consoleErrorSpy).toHaveBeenCalledWith(BUTTON_ERROR_MESSAGE);
+
+    consoleErrorSpy.mockRestore();
+  });
+
+  it("should not render buttons section when no buttons exist in context", () => {
+    const stateWithoutButtons = {
+      ...stateWithButtons,
+      buttons: []
+    };
+
+    cleanup();
+    const noButtonsComponent = renderNavbarContainer(stateWithoutButtons);
+
+    const buttonsContainer = noButtonsComponent.queryByTestId("buttons-container");
+    expect(buttonsContainer).not.toBeInTheDocument();
+  });
+});

packages/component-header-footer/src/header/components/HeaderMain/NavbarContainer/index.js

@@ -10,6 +10,9 @@ import { UniversalNavbar } from "../../UniversalNavbar";
 import { Wrapper } from "./index.styles";
 import { NavItem } from "./NavItem";
 
+export const BUTTON_ERROR_MESSAGE =
+  "Header buttons cannot have both an onClick and an href property as this breaks accessibility. Please remove one";
+
 const NavbarContainer = () => {
   const { navTree, mobileNavTree, buttons, breakpoint } = useAppContext();
   const isMobile = useIsMobile(breakpoint);
@@ -19,6 +22,26 @@ const NavbarContainer = () => {
     setItemOpened(prev => (itemOpened === itemId ? undefined : itemId));
   };
 
+  const validateButton = button => {
+    if (button.onClick && button.href) {
+      console.error(BUTTON_ERROR_MESSAGE);
+    }
+  };
+
+  const handleButtonClick = button => () => {
+    trackGAEvent({
+      event: "link",
+      action: "click",
+      name: "onclick",
+      region: "navbar",
+      type: "internal link",
+      section: "main navbar",
+      text: button.text,
+    });
+
+    if (button.onClick) button.onClick();
+  };
+
   const renderItem = (link, index) => {
     const item = { ...link, id: index };
     const genKey = idGenerator(`${link.text}-${index}-`);
@@ -47,23 +70,17 @@ const NavbarContainer = () => {
           )}
           {!!buttons?.length && (
             <form className="buttons-container" data-testid="buttons-container">
-              {buttons?.map(button => (
-                <Button
-                  {...button}
-                  key={button.text}
-                  onClick={() =>
-                    trackGAEvent({
-                      event: "link",
-                      action: "click",
-                      name: "onclick",
-                      region: "navbar",
-                      type: "internal link",
-                      section: "main navbar",
-                      text: button.text,
-                    })
-                  }
-                />
-              ))}
+              {buttons.map(button => {
+                validateButton(button);
+                console.log(button);
+                return (
+                  <Button
+                    {...button}
+                    key={button.text}
+                    onClick={handleButtonClick(button)}
+                  />
+                );
+              })}
             </form>
           )}
         </div>

packages/component-header-footer/src/header/core/models/app-prop-types.js

@@ -26,7 +26,7 @@ const LoginPropTypes = {
 const ButtonPropTypes = {
   text: PropTypes.string.isRequired,
   color: PropTypes.oneOf(["gold", "maroon", "light", "dark"]),
-  href: PropTypes.string.isRequired,
+  href: PropTypes.string,
   classes: PropTypes.string,
   onClick: PropTypes.func,
   onFocus: PropTypes.func,