Skip to content

Commit 5f52c97

Browse files
mlsamuelsonmlsamuelson
authored
Merge pull request #1494 from ASU/uds-1976
fix(component-header-footer): pass onClick handler to header buttons
2 parents 8932873 + 064292f commit 5f52c97

File tree

4 files changed

+193
-18
lines changed

4 files changed

+193
-18
lines changed

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ const Button = ({ href, color, text, classes, onClick, onFocus }) => {
2020
className={`button-${color} ${classes ?? ""}`}
2121
onClick={onClick}
2222
onFocus={onFocus}
23+
{...(onClick && { role: "button" })}
2324
>
2425
{text}
2526
</ButtonWrapper>

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

Lines changed: 157 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
/* eslint-disable jest/no-mocks-import */
2+
// @ts-check
3+
import { render, fireEvent, cleanup } from "@testing-library/react";
4+
import React from "react";
5+
import { trackGAEvent } from "../../../../../../../shared";
6+
7+
jest.mock("../../../../../../../shared", () => ({
8+
idGenerator: jest.fn().mockImplementation(function* () {
9+
yield "test-id";
10+
}),
11+
trackGAEvent: jest.fn(),
12+
}));
13+
14+
import { NavbarContainer, BUTTON_ERROR_MESSAGE } from ".";
15+
import { AppContextProvider } from "../../../core/context/app-context";
16+
17+
const stateWithButtons = {
18+
navTree: [
19+
{
20+
href: "/",
21+
text: "Home",
22+
type: "icon-home",
23+
class: "home",
24+
},
25+
{
26+
text: "Link option 1",
27+
href: "#",
28+
items: [
29+
[
30+
{
31+
href: "https://www.asu.edu",
32+
text: "Sublink 1",
33+
},
34+
{
35+
href: "https://www.asu.edu",
36+
text: "Sublink 2",
37+
},
38+
{
39+
href: "https://www.asu.edu",
40+
text: "Sublink 3",
41+
},
42+
{
43+
href: "https://www.asu.edu",
44+
text: "Sublink 4",
45+
},
46+
{
47+
href: "https://www.asu.edu",
48+
text: "Sublink 5",
49+
},
50+
{
51+
href: "https://www.asu.edu",
52+
text: "Sublink 6",
53+
},
54+
{
55+
href: "https://www.asu.edu",
56+
text: "Sublink 7",
57+
},
58+
{
59+
href: "https://www.asu.edu",
60+
text: "Sublink 8",
61+
},
62+
],
63+
],
64+
},
65+
],
66+
buttons: [
67+
{ text: "Login", href: "/login" },
68+
{ text: "Sign Up", onClick: jest.fn() },
69+
{ text: "Contact", onClick: jest.fn(), href: "#" }
70+
],
71+
breakpoint: "lg"
72+
};
73+
74+
const renderNavbarContainer = (props) => {
75+
return render(
76+
<AppContextProvider initialValue={props}>
77+
<NavbarContainer />
78+
</AppContextProvider>
79+
);
80+
};
81+
82+
describe("#NavbarContainer Button Tests", () => {
83+
/** @type {import("@testing-library/react").RenderResult} */
84+
let component;
85+
86+
beforeEach(() => {
87+
jest.clearAllMocks();
88+
component = renderNavbarContainer(stateWithButtons);
89+
});
90+
91+
afterEach(cleanup);
92+
93+
it("should render all buttons from context", () => {
94+
const buttonsContainer = component.getByTestId("buttons-container");
95+
expect(buttonsContainer).toBeInTheDocument();
96+
97+
const buttons = component.getAllByRole("button");
98+
expect(buttons.length).toBe(stateWithButtons.buttons.length);
99+
100+
stateWithButtons.buttons.forEach((buttonData, index) => {
101+
expect(buttons[index]).toHaveTextContent(buttonData.text);
102+
});
103+
});
104+
105+
it("should call the onClick handler when a button is clicked", () => {
106+
const buttons = component.getAllByRole("button");
107+
108+
fireEvent.click(buttons[1]);
109+
110+
expect(stateWithButtons.buttons[1].onClick).toHaveBeenCalledTimes(1);
111+
});
112+
113+
it("should track GA event when any button is clicked", () => {
114+
const buttons = component.getAllByRole("button");
115+
116+
buttons.forEach((button, index) => {
117+
fireEvent.click(button);
118+
119+
expect(trackGAEvent).toHaveBeenCalledWith({
120+
event: "link",
121+
action: "click",
122+
name: "onclick",
123+
region: "navbar",
124+
type: "internal link",
125+
section: "main navbar",
126+
text: stateWithButtons.buttons[index].text,
127+
});
128+
});
129+
130+
expect(trackGAEvent).toHaveBeenCalledTimes(buttons.length);
131+
});
132+
133+
it("should log an error for buttons with both onClick and href", () => {
134+
cleanup();
135+
136+
const consoleErrorSpy = jest.spyOn(console, "error").mockImplementation(() => {});
137+
138+
renderNavbarContainer(stateWithButtons);
139+
140+
expect(consoleErrorSpy).toHaveBeenCalledWith(BUTTON_ERROR_MESSAGE);
141+
142+
consoleErrorSpy.mockRestore();
143+
});
144+
145+
it("should not render buttons section when no buttons exist in context", () => {
146+
const stateWithoutButtons = {
147+
...stateWithButtons,
148+
buttons: []
149+
};
150+
151+
cleanup();
152+
const noButtonsComponent = renderNavbarContainer(stateWithoutButtons);
153+
154+
const buttonsContainer = noButtonsComponent.queryByTestId("buttons-container");
155+
expect(buttonsContainer).not.toBeInTheDocument();
156+
});
157+
});

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

Lines changed: 34 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@ import { UniversalNavbar } from "../../UniversalNavbar";
1010
import { Wrapper } from "./index.styles";
1111
import { NavItem } from "./NavItem";
1212

13+
export const BUTTON_ERROR_MESSAGE =
14+
"Header buttons cannot have both an onClick and an href property as this breaks accessibility. Please remove one";
15+
1316
const NavbarContainer = () => {
1417
const { navTree, mobileNavTree, buttons, breakpoint } = useAppContext();
1518
const isMobile = useIsMobile(breakpoint);
@@ -19,6 +22,26 @@ const NavbarContainer = () => {
1922
setItemOpened(prev => (itemOpened === itemId ? undefined : itemId));
2023
};
2124

25+
const validateButton = button => {
26+
if (button.onClick && button.href) {
27+
console.error(BUTTON_ERROR_MESSAGE);
28+
}
29+
};
30+
31+
const handleButtonClick = button => () => {
32+
trackGAEvent({
33+
event: "link",
34+
action: "click",
35+
name: "onclick",
36+
region: "navbar",
37+
type: "internal link",
38+
section: "main navbar",
39+
text: button.text,
40+
});
41+
42+
if (button.onClick) button.onClick();
43+
};
44+
2245
const renderItem = (link, index) => {
2346
const item = { ...link, id: index };
2447
const genKey = idGenerator(`${link.text}-${index}-`);
@@ -47,23 +70,17 @@ const NavbarContainer = () => {
4770
)}
4871
{!!buttons?.length && (
4972
<form className="buttons-container" data-testid="buttons-container">
50-
{buttons?.map(button => (
51-
<Button
52-
{...button}
53-
key={button.text}
54-
onClick={() =>
55-
trackGAEvent({
56-
event: "link",
57-
action: "click",
58-
name: "onclick",
59-
region: "navbar",
60-
type: "internal link",
61-
section: "main navbar",
62-
text: button.text,
63-
})
64-
}
65-
/>
66-
))}
73+
{buttons.map(button => {
74+
validateButton(button);
75+
console.log(button);
76+
return (
77+
<Button
78+
{...button}
79+
key={button.text}
80+
onClick={handleButtonClick(button)}
81+
/>
82+
);
83+
})}
6784
</form>
6885
)}
6986
</div>

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ const LoginPropTypes = {
2626
const ButtonPropTypes = {
2727
text: PropTypes.string.isRequired,
2828
color: PropTypes.oneOf(["gold", "maroon", "light", "dark"]),
29-
href: PropTypes.string.isRequired,
29+
href: PropTypes.string,
3030
classes: PropTypes.string,
3131
onClick: PropTypes.func,
3232
onFocus: PropTypes.func,

0 commit comments

Comments
 (0)