feat(Table): new sort behavior (#1935)

Co-authored-by: Tal Koren <[email protected]>
mondaycom · Mar 6, 2024 · 32790f4 · 32790f4
1 parent 84a0013
commit 32790f4
Show file tree

Hide file tree

Showing 9 changed files with 134 additions and 63 deletions.
diff --git a/packages/core/src/components/Button/Button.tsx b/packages/core/src/components/Button/Button.tsx
@@ -71,6 +71,10 @@ export interface ButtonProps extends VibeComponentProps {
   /** aria controls - receives id for the controlled region */
   ariaControls?: string;
   "aria-describedby"?: AriaAttributes["aria-describedby"];
+  /**
+   * aria to be used for screen reader to know if the button is hidden
+   */
+  "aria-hidden"?: AriaAttributes["aria-hidden"];
   /** On Button Focus callback */
   onFocus?: (event: React.FocusEvent<HTMLButtonElement>) => void;
   /** On Button Blur callback */
@@ -136,6 +140,7 @@ const Button: VibeComponent<ButtonProps, unknown> & {
       ariaExpanded,
       ariaControls,
       "aria-describedby": ariaDescribedBy,
+      "aria-hidden": ariaHidden,
       blurOnMouseUp,
       dataTestId: backwardCompatabilityDataTestId,
       "data-testid": dataTestId,
@@ -250,7 +255,7 @@ const Button: VibeComponent<ButtonProps, unknown> & {
         id,
         onFocus,
         onBlur,
-        tabIndex: disabled ? -1 : tabIndex,
+        tabIndex: disabled || ariaHidden ? -1 : tabIndex,
         "data-testid": overrideDataTestId || getTestId(ComponentDefaultTestId.BUTTON, id),
         onMouseDown: onMouseDownClicked,
         "aria-disabled": disabled,
@@ -260,7 +265,8 @@ const Button: VibeComponent<ButtonProps, unknown> & {
         "aria-haspopup": ariaHasPopup,
         "aria-expanded": ariaExpanded,
         "aria-controls": ariaControls,
-        "aria-describedby": ariaDescribedBy
+        "aria-describedby": ariaDescribedBy,
+        "aria-hidden": ariaHidden
       };
       return props;
     }, [
@@ -284,7 +290,8 @@ const Button: VibeComponent<ButtonProps, unknown> & {
       ariaHasPopup,
       ariaExpanded,
       ariaControls,
-      ariaDescribedBy
+      ariaDescribedBy,
+      ariaHidden
     ]);
 
     const leftIconSize = useMemo(() => {

diff --git a/packages/core/src/components/Button/__tests__/button.jest.js b/packages/core/src/components/Button/__tests__/button.jest.js
@@ -1,33 +1,30 @@
 import React from "react";
 import { render, fireEvent, cleanup, waitFor } from "@testing-library/react";
+import "@testing-library/jest-dom";
 import Button from "../Button";
 
 const text = "Click Me!";
-const className = "test-class";
-
-function renderComponent(props) {
-  return render(<Button {...props}>{text}</Button>);
-}
-
-describe("<Buttoon />", () => {
-  let clickActionStub;
-  let onMouseDownStub;
-  let buttonComponent;
-
-  beforeEach(() => {
-    clickActionStub = jest.fn();
-    onMouseDownStub = jest.fn();
-    buttonComponent = render(
-      <Button className={className} onClick={clickActionStub} onMouseDown={onMouseDownStub}>
-        {text}
-      </Button>
-    );
-  });
 
+describe("<Button />", () => {
   afterEach(() => {
     cleanup();
   });
+
   describe("click", () => {
+    let clickActionStub;
+    let onMouseDownStub;
+    let buttonComponent;
+
+    beforeEach(() => {
+      clickActionStub = jest.fn();
+      onMouseDownStub = jest.fn();
+      buttonComponent = render(
+        <Button onClick={clickActionStub} onMouseDown={onMouseDownStub}>
+          {text}
+        </Button>
+      );
+    });
+
     it("should call the click callback when clicked", () => {
       const { getByText } = buttonComponent;
       fireEvent.click(getByText(text));
@@ -99,17 +96,19 @@ describe("<Buttoon />", () => {
 
   it("should call on blur", () => {
     const onBlur = jest.fn();
-    cleanup();
-    const { getByText } = renderComponent({ onBlur });
+    const { getByText } = render(<Button onBlur={onBlur}>{text}</Button>);
     const button = getByText(text);
     fireEvent.blur(button);
     expect(onBlur.mock.calls.length).toEqual(1);
   });
 
   it("should call do blur on mouseup", () => {
     const onBlur = jest.fn();
-    cleanup();
-    const { getByText } = renderComponent({ onBlur, blurOnMouseUp: false });
+    const { getByText } = render(
+      <Button onBlur={onBlur} blurOnMouseUp={false}>
+        {text}
+      </Button>
+    );
     const button = getByText(text);
     fireEvent.focus(button);
     fireEvent.mouseUp(button);
@@ -118,43 +117,56 @@ describe("<Buttoon />", () => {
 
   it("should call on focus", () => {
     const onFocus = jest.fn();
-    cleanup();
-    const { getByText } = renderComponent({ onFocus });
+    const { getByText } = render(<Button onFocus={onFocus}>{text}</Button>);
     const button = getByText(text);
     fireEvent.focus(button);
     expect(onFocus.mock.calls.length).toEqual(1);
   });
 
   describe("mouse down", () => {
+    const onClick = jest.fn();
     it("should call the click callback when clicked", () => {
-      const { getByText } = buttonComponent;
+      const { getByText } = render(<Button onClick={onClick}>{text}</Button>);
       fireEvent.mouseDown(getByText(text));
-      expect(clickActionStub.mock.calls.length).toEqual(0);
+      expect(onClick).not.toBeCalled();
     });
 
     it("should call mouse down callback", () => {
-      const { getByText, rerender } = buttonComponent;
       const onMouseDown = jest.fn();
-      rerender(
-        <Button onClick={clickActionStub} onMouseDown={onMouseDown}>
+      const { getByText } = render(
+        <Button onClick={onClick} onMouseDown={onMouseDown}>
           {text}
         </Button>
       );
       fireEvent.mouseDown(getByText(text));
       expect(onMouseDown.mock.calls.length).toEqual(1);
     });
   });
+
   describe("a11y", () => {
     it("Aria label should be connected to the button", () => {
       const ariaLabel = "Icon Name";
+      const onClick = jest.fn();
       const { getByLabelText } = render(
-        <Button ariaLabel={ariaLabel} className={className} onClick={clickActionStub} onMouseDown={onMouseDownStub}>
+        <Button ariaLabel={ariaLabel} onClick={onClick}>
           {text}
         </Button>
       );
       const buttonElement = getByLabelText(ariaLabel);
       fireEvent.click(buttonElement);
-      expect(clickActionStub.mock.calls.length).toEqual(1);
+      expect(onClick.mock.calls.length).toEqual(1);
+    });
+
+    it("should apply aria-hidden attribute to button", () => {
+      const { getByRole } = render(<Button aria-hidden>{text}</Button>);
+      const buttonElement = getByRole("button", { hidden: true });
+      expect(buttonElement).toHaveAttribute("aria-hidden", "true");
+    });
+
+    it("should not apply aria-hidden attribute to button", () => {
+      const { getByRole } = render(<Button>{text}</Button>);
+      const buttonElement = getByRole("button");
+      expect(buttonElement).not.toHaveAttribute("aria-hidden");
     });
   });
 });
diff --git a/packages/core/src/components/IconButton/IconButton.tsx b/packages/core/src/components/IconButton/IconButton.tsx
@@ -1,4 +1,4 @@
-import React, { forwardRef, Fragment, useMemo, useRef } from "react";
+import React, { AriaAttributes, forwardRef, Fragment, useMemo, useRef } from "react";
 import cx from "classnames";
 import { noop as NOOP } from "lodash-es";
 import useMergeRef from "../../hooks/useMergeRef";
@@ -43,6 +43,10 @@ export interface IconButtonProps extends VibeComponentProps {
    * a11y property to be added, used for screen reader to know if the button is expanded
    */
   ariaExpanded?: boolean;
+  /**
+   * a11y property to be added, used for screen reader to know if the button is hidden
+   */
+  "aria-hidden"?: AriaAttributes["aria-hidden"];
   /**
    * Size of the icon
    */
@@ -101,6 +105,7 @@ const IconButton: VibeComponent<IconButtonProps> & {
       tooltipContent,
       ariaLabel,
       ariaExpanded,
+      "aria-hidden": ariaHidden,
       hideTooltip = false,
       kind = IconButton.kinds.TERTIARY,
       active,
@@ -183,6 +188,7 @@ const IconButton: VibeComponent<IconButtonProps> & {
             kind={kind}
             ariaLabel={buttonAriaLabel}
             ariaExpanded={ariaExpanded}
+            aria-hidden={ariaHidden}
             ref={mergedRef}
             id={id}
             data-testid={overrideDataTestId || getTestId(ComponentDefaultTestId.ICON_BUTTON, id)}

diff --git a/packages/core/src/components/IconButton/__tests__/iconButton-tests.jest.tsx b/packages/core/src/components/IconButton/__tests__/iconButton-tests.jest.tsx
@@ -1,12 +1,15 @@
 import React from "react";
 import { fireEvent, render, cleanup, screen } from "@testing-library/react";
+import "@testing-library/jest-dom";
 import { act } from "@testing-library/react-hooks";
 import IconButton, { IconButtonProps } from "../IconButton";
 
 jest.useFakeTimers();
 
-const renderComponent = (props: IconButtonProps) => {
-  return render(<IconButton {...props} />);
+const ariaLabel = "Button Icon";
+
+const renderComponent = (props: IconButtonProps = {}) => {
+  return render(<IconButton ariaLabel={ariaLabel} {...props} />);
 };
 
 describe("IconButton tests", () => {
@@ -17,29 +20,26 @@ describe("IconButton tests", () => {
   describe("click", () => {
     it("should call the callback function when clicked ", () => {
       const onClick = jest.fn();
-      const ariaLabel = "Button Icon";
-      renderComponent({ onClick, ariaLabel });
+      renderComponent({ onClick });
       const component = screen.getByLabelText(ariaLabel);
       fireEvent.click(component);
       expect(onClick.mock.calls.length).toBe(1);
     });
 
     it("should not call the callback if disabled when clicked ", () => {
       const onClick = jest.fn();
-      const ariaLabel = "Button Icon";
-      renderComponent({ onClick, ariaLabel, disabled: true });
+      renderComponent({ onClick, disabled: true });
       const component = screen.getByLabelText(ariaLabel);
       fireEvent.click(component);
       expect(onClick.mock.calls.length).toBe(0);
     });
   });
 
   describe("Tooltips", () => {
-    it("should display the tooltip content", () => {
+    it("should display the tooltip content", async () => {
       const tooltipContent = "My Text";
-      const ariaLabel = "Button Icon";
 
-      renderComponent({ tooltipContent, ariaLabel });
+      renderComponent({ tooltipContent });
       const component = screen.getByLabelText(ariaLabel);
       act(() => {
         fireEvent.mouseEnter(component);
@@ -54,9 +54,7 @@ describe("IconButton tests", () => {
     });
 
     it("should display the tooltip with aria label", () => {
-      const ariaLabel = "Button Icon";
-
-      renderComponent({ ariaLabel });
+      renderComponent();
       const component = screen.getByLabelText(ariaLabel);
       act(() => {
         fireEvent.mouseEnter(component);
@@ -71,10 +69,9 @@ describe("IconButton tests", () => {
     });
 
     it("should display not disabledReason if disabled is false", () => {
-      const ariaLabel = "Button Icon";
       const disabledReason = "I'm a disabled button";
 
-      renderComponent({ ariaLabel, disabledReason });
+      renderComponent({ disabledReason });
       const component = screen.getByLabelText(ariaLabel);
       act(() => {
         fireEvent.mouseEnter(component);
@@ -89,10 +86,9 @@ describe("IconButton tests", () => {
     });
 
     it("should display disabledReason if disabled is true", () => {
-      const ariaLabel = "Button Icon";
       const disabledReason = "I'm a disabled button";
 
-      renderComponent({ ariaLabel, disabledReason, disabled: true });
+      renderComponent({ disabledReason, disabled: true });
       const component = screen.getByLabelText(ariaLabel);
       act(() => {
         fireEvent.mouseEnter(component);
@@ -106,4 +102,20 @@ describe("IconButton tests", () => {
       jest.advanceTimersByTime(1000);
     });
   });
+
+  describe("a11y", () => {
+    describe("aria-hidden", () => {
+      it("should pass aria-hidden attribute to Button component", () => {
+        const { getByRole } = renderComponent({ "aria-hidden": true });
+        const buttonComponent = getByRole("button", { hidden: true });
+        expect(buttonComponent).toHaveAttribute("aria-hidden", "true");
+      });
+
+      it("should not have aria-hidden attribute on Button component if not specified", () => {
+        const { getByRole } = renderComponent();
+        const buttonComponent = getByRole("button");
+        expect(buttonComponent).not.toHaveAttribute("aria-hidden");
+      });
+    });
+  });
 });
diff --git a/packages/core/src/components/Table/Table/__tests__/table.jest.tsx b/packages/core/src/components/Table/Table/__tests__/table.jest.tsx
@@ -201,12 +201,20 @@ describe("Table", () => {
     describe.each([
       ["asc", "ascending"],
       ["desc", "descending"],
-      ["none", null]
+      ["none", "none"]
     ])("Sort", (sortState: ITableHeaderCellProps["sortState"], ariaSort) => {
-      it(`Should apply aria-sort to header element (${sortState}, ${ariaSort})`, () => {
-        const { getByRole } = render(<TableHeaderCell title="Title" sortState={sortState} />);
+      it(`Should apply aria-sort to header element (${sortState}, ${ariaSort}) when onSortClicked is defined`, () => {
+        const onSortClicked = jest.fn();
+        const { getByRole } = render(
+          <TableHeaderCell title="Title" sortState={sortState} onSortClicked={onSortClicked} />
+        );
         expect(getByRole("columnheader").getAttribute("aria-sort")).toBe(ariaSort);
       });
+
+      it(`Should not apply aria-sort to header element (${sortState}, ${ariaSort}) when onSortClicked isn't defined`, () => {
+        const { getByRole } = render(<TableHeaderCell title="Title" sortState={sortState} />);
+        expect(getByRole("columnheader")).not.toHaveAttribute("aria-sort");
+      });
     });
   });
 });
diff --git a/packages/core/src/components/Table/Table/tableHelpers.ts b/packages/core/src/components/Table/Table/tableHelpers.ts
@@ -48,9 +48,11 @@ export function getNextSortState(sortState: ITableHeaderCellProps["sortState"]):
 export function getAriaSort(sortState: ITableHeaderCellProps["sortState"]): AriaAttributes["aria-sort"] {
   if (sortState === "asc") {
     return "ascending";
-  } else if (sortState === "desc") {
+  }
+  if (sortState === "desc") {
     return "descending";
   }
+  return "none";
 }
 
 export function getSkeletonType(loadingStateType: TableLoadingStateType): SkeletonType {