Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,23 @@ export const Default: Story = {};
export const Submitting: Story = {
play: async ({ args }) => {
const button = await screen.findByRole('button', { name: 'Submit' });
await expect(button).toBeDisabled();
await expect(button).toHaveAttribute('aria-disabled', true);

await userEvent.click(await screen.findByText('Design system'));
await expect(button).toBeDisabled();
await expect(button).toHaveAttribute('aria-disabled', true);

await userEvent.click(await screen.findByText('Functional testing'));
await userEvent.click(await screen.findByText('Accessibility testing'));
await userEvent.click(await screen.findByText('Visual testing'));
await expect(button).toBeDisabled();
await expect(button).toHaveAttribute('aria-disabled', true);

await userEvent.selectOptions(screen.getByRole('combobox'), ['We use it at work']);
await expect(button).not.toBeDisabled();
await expect(button).not.toHaveAttribute('aria-disabled', true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

logic: The assertion not.toHaveAttribute('aria-disabled', true) may not work as expected. When aria-disabled is false, the attribute might be present with value 'false' rather than absent. Consider using toHaveAttribute('aria-disabled', 'false') or checking if the attribute is absent entirely.

Suggested change
await expect(button).not.toHaveAttribute('aria-disabled', true);
await expect(button).toHaveAttribute('aria-disabled', 'false');


await userEvent.click(button);

await waitFor(async () => {
await expect(button).toBeDisabled();
await expect(button).toHaveAttribute('aria-disabled', true);
await expect(args.onComplete).toHaveBeenCalledWith({
building: {
'application-ui': false,
Expand Down
12 changes: 10 additions & 2 deletions code/core/src/components/components/Button/Button.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { styled } from 'storybook/internal/theming';
import { FaceHappyIcon } from '@storybook/icons';

import preview from '../../../../../.storybook/preview';
import { Button } from './Button';
import { Button, type ButtonProps } from './Button';

const meta = preview.meta({
id: 'button-component',
Expand Down Expand Up @@ -158,7 +158,15 @@ export const Disabled = meta.story({
args: {
disabled: true,
children: 'Disabled Button',
},
onClick: () => {},
} satisfies ButtonProps,
render: (args) => (
<Row>
<Button variant="solid" {...args}>
Disabled Button
</Button>
</Row>
),
});

export const WithHref = meta.story({
Expand Down
16 changes: 13 additions & 3 deletions code/core/src/components/components/Button/Button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { Slot } from '@radix-ui/react-slot';
import { darken, lighten, rgba, transparentize } from 'polished';
import { isPropValid, styled } from 'storybook/theming';

export interface ButtonProps extends ButtonHTMLAttributes<HTMLButtonElement> {
export interface ButtonProps extends Omit<ButtonHTMLAttributes<HTMLButtonElement>, 'disabled'> {
asChild?: boolean;
size?: 'small' | 'medium';
padding?: 'small' | 'medium' | 'none';
Expand All @@ -25,9 +25,10 @@ export const Button = forwardRef<HTMLButtonElement, ButtonProps>(
variant = 'outline',
padding = 'medium',
disabled = false,
disabled = false,
active = false,
onClick,
...props
...restProps
},
ref
) => {
Expand All @@ -40,6 +41,11 @@ export const Button = forwardRef<HTMLButtonElement, ButtonProps>(
const [isAnimating, setIsAnimating] = useState(false);

const handleClick = (event: SyntheticEvent) => {
if (disabled) {
event.preventDefault();
event.stopPropagation();
return;
}
if (onClick) {
onClick(event);
}
Expand All @@ -66,12 +72,13 @@ export const Button = forwardRef<HTMLButtonElement, ButtonProps>(
variant={variant}
size={size}
padding={padding}
aria-disabled={disabled}
disabled={disabled}
active={active}
animating={isAnimating}
animation={animation}
onClick={handleClick}
{...props}
{...restProps}
/>
);
}
Expand All @@ -86,9 +93,11 @@ const StyledButton = styled('button', {
animating: boolean;
animation: ButtonProps['animation'];
}
>(({ theme, variant, size, active, disabled, animating, animation = 'none', padding }) => ({
>(({ theme, variant, size, disabled, active, animating, animation = 'none', padding }) => ({
border: 0,
cursor: disabled ? 'not-allowed' : 'pointer',
cursor: disabled ? 'not-allowed' : 'pointer',
display: 'inline-flex',
gap: '6px',
alignItems: 'center',
Expand Down Expand Up @@ -123,6 +132,7 @@ const StyledButton = styled('button', {
whiteSpace: 'nowrap',
userSelect: 'none',
opacity: disabled ? 0.5 : 1,
opacity: disabled ? 0.5 : 1,
margin: 0,
fontSize: `${theme.typography.size.s1}px`,
fontWeight: theme.typography.weight.bold,
Expand Down
2 changes: 1 addition & 1 deletion code/e2e-tests/addon-toolbars.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,6 @@ test.describe('addon-toolbars', () => {
await expect(sbPage.previewRoot()).toContainText('안녕하세요');

const button = sbPage.page.getByTitle('Internationalization locale');
await expect(button).toBeDisabled();
await expect(button).toHaveAttribute('aria-disabled');
});
});
10 changes: 5 additions & 5 deletions code/e2e-tests/addon-viewport.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,16 +30,16 @@ test.describe('addon-viewport', () => {

// Measure the original dimensions of previewRoot
const originalDimensions = await sbPage.getCanvasBodyElement().boundingBox();
await expect(originalDimensions?.width).toBeDefined();
expect(originalDimensions?.width).toBeDefined();

await sbPage.selectToolbar('[title="Change the size of the preview"]', '#list-item-mobile1');

// Measure the adjusted dimensions of previewRoot after clicking the mobile item.
const adjustedDimensions = await sbPage.getCanvasBodyElement().boundingBox();
await expect(adjustedDimensions?.width).toBeDefined();
expect(adjustedDimensions?.width).toBeDefined();

// Compare the two widths
await expect(adjustedDimensions?.width).not.toBe(originalDimensions?.width);
expect(adjustedDimensions?.width).not.toBe(originalDimensions?.width);
});

test('viewport should be uneditable when a viewport is set via globals', async ({ page }) => {
Expand All @@ -50,10 +50,10 @@ test.describe('addon-viewport', () => {

// Measure the original dimensions of previewRoot
const originalDimensions = await sbPage.getCanvasBodyElement().boundingBox();
await expect(originalDimensions?.width).toBeDefined();
expect(originalDimensions?.width).toBeDefined();

const toolbar = page.getByTitle('Change the size of the preview');

await expect(toolbar).toBeDisabled();
await expect(toolbar).toHaveAttribute('aria-disabled');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style: Consider adding a more specific assertion to check that the attribute value is 'true' rather than just checking for its presence

Suggested change
await expect(toolbar).toHaveAttribute('aria-disabled');
await expect(toolbar).toHaveAttribute('aria-disabled', 'true');

});
});
Original file line number Diff line number Diff line change
Expand Up @@ -214,9 +214,8 @@ test.describe("component testing", () => {

// Wait for both the watch mode button to be disabled and the testing text to appear
await Promise.all([
expect(watchModeButton).toBeDisabled(),
expect(watchModeButton).toHaveAttribute("aria-disabled", true),
expect(page.locator("#testing-module-description")).toHaveText(/Testing/),

]);

// Wait for test results to appear
Expand Down