Skip to content

Commit

Permalink
feat: allow focusing disabled buttons and displaying tooltips (#1082)
Browse files Browse the repository at this point in the history
Signed-off-by: Peter Makowski <me+git@petermakowski.io>
  • Loading branch information
petermakowski committed May 13, 2024
1 parent 2ab85fd commit 929a409
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 20 deletions.
20 changes: 20 additions & 0 deletions src/components/Button/Button.stories.mdx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { ArgsTable, Canvas, Meta, Story } from "@storybook/addon-docs";

import Button, { ButtonAppearance } from "./Button";
import Tooltip from "../Tooltip";

<Meta
title="Button"
Expand Down Expand Up @@ -231,3 +232,22 @@ Buttons are clickable elements used to perform an action, they can be used for b
{Template.bind({})}
</Story>
</Canvas>

<Canvas>
<Story
name="Disabled with tooltip"
args={{
children: "Disabled button with a tooltip",
disabled: true,
}}
>
{(args) => (
<div style={{ paddingTop: "3rem" }}>
<Tooltip message="This button is disabled">
<Button {...args} />
</Tooltip>
</div>
)}
</Story>
</Canvas>

15 changes: 4 additions & 11 deletions src/components/Button/Button.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,21 +37,14 @@ describe("Button ", () => {
const onClick = jest.fn();
render(<Button disabled={true} onClick={onClick} />);
const button = screen.getByRole("button");
expect(button).toBeDisabled();
expect(button).not.toHaveAttribute("aria-disabled");
expect(button).not.toHaveClass("is-disabled");
// The button should be disabled but not have the disabled attribute to allow for focus and displaying the tooltip
expect(button).not.toBeDisabled();
expect(button).toHaveAttribute("aria-disabled");
expect(button).toHaveClass("is-disabled");
await userEvent.click(button);
expect(onClick).not.toHaveBeenCalled();
});

it("does not prevent default when disabling a button", async () => {
render(<Button disabled={true} onClick={jest.fn()} />);
const button = screen.getByRole("button");
const clickEvent = createEvent.click(button);
fireEvent(button, clickEvent);
expect(clickEvent.defaultPrevented).toBe(false);
});

it("correctly disables a link", async () => {
const onClick = jest.fn();
render(
Expand Down
6 changes: 2 additions & 4 deletions src/components/Button/Button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -88,21 +88,19 @@ const Button = <P,>({
{
"has-icon": hasIcon,
"is-dense": dense,
"is-disabled": Component !== "button" && disabled,
"is-disabled": disabled,
"is-inline": inline,
"is-small": small,
},
className
);
const onClickDisabled = (e: MouseEvent) => e.preventDefault();
const disabledProp =
Component === "button" ? { disabled } : { "aria-disabled": disabled };

return (
<Component
className={classes}
onClick={disabled ? onClickDisabled : onClick}
{...disabledProp}
aria-disabled={disabled || undefined}
{...buttonProps}
>
{children}
Expand Down
4 changes: 3 additions & 1 deletion src/components/ContextualMenu/ContextualMenu.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ describe("ContextualMenu ", () => {

it("can be disabled", () => {
render(<ContextualMenu links={[]} toggleDisabled toggleLabel="Toggle" />);
expect(screen.getByRole("button", { name: "Toggle" })).toBeDisabled();
expect(screen.getByRole("button", { name: "Toggle" })).toHaveAttribute(
"aria-disabled"
);
});

it("can have a toggle button with just an icon", () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ exports[`<TablePagination /> renders table pagination and matches the snapshot 1
Showing all 5 items
</div>
<button
aria-disabled="true"
aria-label="Previous page"
class="p-button--base has-icon back"
disabled=""
class="p-button--base has-icon is-disabled back"
>
<i
class="p-icon--chevron-down"
Expand Down Expand Up @@ -45,9 +45,9 @@ exports[`<TablePagination /> renders table pagination and matches the snapshot 1
of 
1
<button
aria-disabled="true"
aria-label="Next page"
class="p-button--base has-icon next"
disabled=""
class="p-button--base has-icon is-disabled next"
>
<i
class="p-icon--chevron-down"
Expand Down

0 comments on commit 929a409

Please sign in to comment.