Skip to content

Commit 55724aa

Browse files
gcornutgcornut
committed
refactor(components): uniformize handling of link and button
1 parent d8f7295 commit 55724aa

File tree

15 files changed

+321
-138
lines changed

15 files changed

+321
-138
lines changed

‎CHANGELOG.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1616
- Import types and constants from `@lumx/core` and re-export
1717
- Import `className` utilities from `@lumx/core`
1818
- Migrate tests to `vitest`
19+
- Uniformize link and button handling in `Link`, `Button`, `SideNavigationItem`, `Thumbnail` and `NavigationItem`.
20+
- Render disabled links as link instead of disabled buttons.
1921

2022
## [3.19.0][] - 2025-11-07
2123

‎packages/lumx-react/src/components/button/Button.test.tsx‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -78,9 +78,10 @@ describe(`<${Button.displayName}>`, () => {
7878
it('should render disabled link', async () => {
7979
const onClick = vi.fn();
8080
const { button } = setup({ children: 'Label', disabled: true, href: 'https://example.com', onClick });
81-
// Disabled link do not exist so we fallback to a button
82-
expect(screen.queryByRole('link')).not.toBeInTheDocument();
83-
expect(button).toHaveAttribute('disabled');
81+
expect(screen.queryByRole('link')).toBeInTheDocument();
82+
expect(button).toHaveAttribute('aria-disabled', 'true');
83+
// Simulate standard disabled state (not focusable)
84+
expect(button).toHaveAttribute('tabindex', '-1');
8485
await userEvent.click(button);
8586
expect(onClick).not.toHaveBeenCalled();
8687
});
@@ -102,8 +103,7 @@ describe(`<${Button.displayName}>`, () => {
102103
onClick,
103104
});
104105
expect(button).toHaveAccessibleName('Label');
105-
// Disabled link do not exist so we fallback to a button
106-
expect(screen.queryByRole('link')).not.toBeInTheDocument();
106+
expect(screen.queryByRole('link')).toBeInTheDocument();
107107
expect(button).toHaveAttribute('aria-disabled', 'true');
108108
await userEvent.click(button);
109109
expect(onClick).not.toHaveBeenCalled();

‎packages/lumx-react/src/components/button/ButtonRoot.tsx‎

Lines changed: 6 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,15 @@
11
import React, { AriaAttributes, ButtonHTMLAttributes, DetailedHTMLProps, RefObject } from 'react';
22

3-
import isEmpty from 'lodash/isEmpty';
4-
53
import classNames from 'classnames';
64

75
import { ColorPalette, Emphasis, Size, Theme } from '@lumx/react';
86
import { CSS_PREFIX } from '@lumx/react/constants';
97
import { GenericProps, HasTheme } from '@lumx/react/utils/type';
108
import { handleBasicClasses } from '@lumx/core/js/utils/className';
11-
import { renderLink } from '@lumx/react/utils/react/renderLink';
129
import { forwardRef } from '@lumx/react/utils/react/forwardRef';
13-
import { useDisableStateProps } from '@lumx/react/utils/disabled/useDisableStateProps';
1410
import { HasAriaDisabled } from '@lumx/react/utils/type/HasAriaDisabled';
11+
import { RawClickable } from '@lumx/react/utils/react/RawClickable';
12+
import { useDisableStateProps } from '@lumx/react/utils/disabled';
1513

1614
type HTMLButtonProps = DetailedHTMLProps<ButtonHTMLAttributes<HTMLButtonElement>, HTMLButtonElement>;
1715

@@ -107,18 +105,14 @@ export const ButtonRoot = forwardRef<ButtonRootProps, HTMLButtonElement | HTMLAn
107105
color,
108106
emphasis,
109107
hasBackground,
110-
href,
111108
isSelected,
112109
isActive,
113110
isFocused,
114111
isHovered,
115112
linkAs,
116-
name,
117113
size,
118-
target,
119114
theme,
120115
variant,
121-
type = 'button',
122116
fullWidth,
123117
...forwardedProps
124118
} = otherProps;
@@ -139,7 +133,7 @@ export const ButtonRoot = forwardRef<ButtonRootProps, HTMLButtonElement | HTMLAn
139133
color: adaptedColor,
140134
emphasis,
141135
isSelected,
142-
isDisabled: isAnyDisabled,
136+
isDisabled: props.isDisabled || props['aria-disabled'],
143137
isActive,
144138
isFocused,
145139
isHovered,
@@ -151,42 +145,18 @@ export const ButtonRoot = forwardRef<ButtonRootProps, HTMLButtonElement | HTMLAn
151145
}),
152146
);
153147

154-
/**
155-
* If the linkAs prop is used, we use the linkAs component instead of a <button>.
156-
* If there is an href attribute, we display an <a> instead of a <button>.
157-
*
158-
* However, in any case, if the component is disabled, we returned a <button> since disabled is not compatible with <a>.
159-
*/
160-
if ((linkAs || !isEmpty(props.href)) && !isAnyDisabled) {
161-
return renderLink(
162-
{
163-
linkAs,
164-
...forwardedProps,
165-
'aria-label': ariaLabel,
166-
href,
167-
target,
168-
className: buttonClassName,
169-
ref: ref as RefObject<HTMLAnchorElement>,
170-
},
171-
children,
172-
);
173-
}
174148
return (
175-
<button
149+
<RawClickable
150+
as={linkAs || (forwardedProps.href ? 'a' : 'button')}
176151
{...forwardedProps}
177152
{...disabledStateProps}
178153
aria-disabled={isAnyDisabled}
179154
aria-label={ariaLabel}
180155
ref={ref as RefObject<HTMLButtonElement>}
181156
className={buttonClassName}
182-
name={name}
183-
type={
184-
// eslint-disable-next-line react/button-has-type
185-
type
186-
}
187157
>
188158
{children}
189-
</button>
159+
</RawClickable>
190160
);
191161
});
192162
ButtonRoot.displayName = COMPONENT_NAME;

‎packages/lumx-react/src/components/link/Link.test.tsx‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -85,17 +85,20 @@ describe(`<${Link.displayName}>`, () => {
8585
it('should render disabled link', async () => {
8686
const onClick = vi.fn();
8787
const { link } = setup({ children: 'Label', isDisabled: true, href: 'https://example.com', onClick });
88-
// Disabled link do not exist so we fallback to a button
89-
expect(screen.queryByRole('link')).not.toBeInTheDocument();
90-
expect(link).toHaveAttribute('disabled');
88+
expect(screen.queryByRole('link')).toBeInTheDocument();
89+
expect(link).toHaveAttribute('aria-disabled');
90+
// Simulate standard disabled state (not focusable)
91+
expect(link).toHaveAttribute('tabindex', '-1');
9192
await userEvent.click(link);
9293
expect(onClick).not.toHaveBeenCalled();
9394
});
9495

9596
it('should render aria-disabled button', async () => {
9697
const onClick = vi.fn();
9798
const { link } = setup({ children: 'Label', 'aria-disabled': true, onClick });
98-
expect(link).toHaveAttribute('aria-disabled');
99+
expect(screen.queryByRole('button')).toBeInTheDocument();
100+
expect(link).toHaveAttribute('aria-disabled', 'true');
101+
expect(link).not.toHaveAttribute('tabindex');
99102
await userEvent.click(link);
100103
expect(onClick).not.toHaveBeenCalled();
101104
});
@@ -109,8 +112,7 @@ describe(`<${Link.displayName}>`, () => {
109112
onClick,
110113
});
111114
expect(link).toHaveAccessibleName('Label');
112-
// Disabled link do not exist so we fallback to a button
113-
expect(screen.queryByRole('link')).not.toBeInTheDocument();
115+
expect(screen.queryByRole('link')).toBeInTheDocument();
114116
expect(link).toHaveAttribute('aria-disabled', 'true');
115117
await userEvent.click(link);
116118
expect(onClick).not.toHaveBeenCalled();

‎packages/lumx-react/src/components/link/Link.tsx‎

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,9 @@ import {
1212
} from '@lumx/core/js/utils/className';
1313
import { forwardRef } from '@lumx/react/utils/react/forwardRef';
1414
import { wrapChildrenIconWithSpaces } from '@lumx/react/utils/react/wrapChildrenIconWithSpaces';
15-
import { useDisableStateProps } from '@lumx/react/utils/disabled/useDisableStateProps';
1615
import { HasAriaDisabled } from '@lumx/react/utils/type/HasAriaDisabled';
16+
import { RawClickable } from '@lumx/react/utils/react/RawClickable';
17+
import { useDisableStateProps } from '@lumx/react/utils/disabled';
1718

1819
type HTMLAnchorProps = React.DetailedHTMLProps<React.AnchorHTMLAttributes<HTMLAnchorElement>, HTMLAnchorElement>;
1920

@@ -67,38 +68,26 @@ const CLASSNAME = getRootClassName(COMPONENT_NAME);
6768
* @return React element.
6869
*/
6970
export const Link = forwardRef<LinkProps, HTMLAnchorElement | HTMLButtonElement>((props, ref) => {
70-
const { isAnyDisabled, disabledStateProps, otherProps } = useDisableStateProps(props);
71+
const { disabledStateProps, otherProps } = useDisableStateProps(props);
7172
const {
7273
children,
7374
className,
7475
color: propColor,
7576
colorVariant: propColorVariant,
76-
href,
7777
leftIcon,
78-
linkAs,
7978
rightIcon,
80-
target,
8179
typography,
80+
linkAs,
8281
...forwardedProps
8382
} = otherProps;
8483
const [color, colorVariant] = resolveColorWithVariants(propColor, propColorVariant);
8584

86-
const isLink = linkAs || href;
87-
const Component = isLink && !isAnyDisabled ? linkAs || 'a' : 'button';
88-
const baseProps: React.ComponentProps<typeof Component> = {};
89-
if (Component === 'button') {
90-
baseProps.type = 'button';
91-
Object.assign(baseProps, disabledStateProps);
92-
} else if (isLink) {
93-
baseProps.href = href;
94-
baseProps.target = target;
95-
}
96-
9785
return (
98-
<Component
99-
ref={ref}
86+
<RawClickable
87+
ref={ref as any}
88+
as={linkAs || (forwardedProps.href ? 'a' : 'button')}
10089
{...forwardedProps}
101-
{...baseProps}
90+
{...disabledStateProps}
10291
className={classNames(
10392
className,
10493
handleBasicClasses({ prefix: CLASSNAME, color, colorVariant, hasTypography: !!typography }),
@@ -112,7 +101,7 @@ export const Link = forwardRef<LinkProps, HTMLAnchorElement | HTMLButtonElement>
112101
{rightIcon && <Icon icon={rightIcon} className={`${CLASSNAME}__right-icon`} />}
113102
</>,
114103
)}
115-
</Component>
104+
</RawClickable>
116105
);
117106
});
118107
Link.displayName = COMPONENT_NAME;

‎packages/lumx-react/src/components/navigation/NavigationItem.tsx‎

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,12 @@
11
import React, { ElementType, ReactNode } from 'react';
22
import { Icon, Placement, Size, Tooltip, Text } from '@lumx/react';
33
import { getRootClassName, handleBasicClasses } from '@lumx/core/js/utils/className';
4-
import { ComponentRef, HasClassName, HasPolymorphicAs, HasTheme } from '@lumx/react/utils/type';
4+
import { ComponentRef, HasClassName, HasPolymorphicAs, HasRequiredLinkHref, HasTheme } from '@lumx/react/utils/type';
55
import classNames from 'classnames';
66
import { forwardRefPolymorphic } from '@lumx/react/utils/react/forwardRefPolymorphic';
77
import { useTheme } from '@lumx/react/utils/theme/ThemeContext';
88
import { useOverflowTooltipLabel } from '@lumx/react/hooks/useOverflowTooltipLabel';
9+
import { RawClickable } from '@lumx/react/utils/react/RawClickable';
910

1011
type BaseNavigationItemProps = {
1112
/** Icon (SVG path). */
@@ -16,17 +17,14 @@ type BaseNavigationItemProps = {
1617
isCurrentPage?: boolean;
1718
};
1819

19-
/** Make `href` required when `as` is `a` */
20-
type RequiredLinkHref<E> = E extends 'a' ? { href: string } : Record<string, unknown>;
21-
2220
/**
2321
* Navigation item props
2422
*/
2523
export type NavigationItemProps<E extends ElementType = 'a'> = HasPolymorphicAs<E> &
2624
HasTheme &
2725
HasClassName &
2826
BaseNavigationItemProps &
29-
RequiredLinkHref<E>;
27+
HasRequiredLinkHref<E>;
3028

3129
/**
3230
* Component display name.
@@ -44,8 +42,6 @@ export const NavigationItem = Object.assign(
4442
const theme = useTheme();
4543
const { tooltipLabel, labelRef } = useOverflowTooltipLabel(label);
4644

47-
const buttonProps = Element === 'button' ? { type: 'button' } : {};
48-
4945
return (
5046
<li
5147
className={classNames(
@@ -57,14 +53,14 @@ export const NavigationItem = Object.assign(
5753
)}
5854
>
5955
<Tooltip label={tooltipLabel} placement={Placement.TOP}>
60-
<Element
56+
<RawClickable
57+
as={Element}
6158
className={handleBasicClasses({
6259
prefix: `${CLASSNAME}__link`,
6360
isSelected: isCurrentPage,
6461
})}
6562
ref={ref as React.Ref<any>}
6663
aria-current={isCurrentPage ? 'page' : undefined}
67-
{...buttonProps}
6864
{...forwardedProps}
6965
>
7066
{icon ? (
@@ -74,7 +70,7 @@ export const NavigationItem = Object.assign(
7470
<Text as="span" truncate className={`${CLASSNAME}__label`} ref={labelRef}>
7571
{label}
7672
</Text>
77-
</Element>
73+
</RawClickable>
7874
</Tooltip>
7975
</li>
8076
);

‎packages/lumx-react/src/components/navigation/NavigationSection.tsx‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { ThemeProvider, useTheme } from '@lumx/react/utils/theme/ThemeContext';
99
import { useId } from '@lumx/react/hooks/useId';
1010
import { forwardRef } from '@lumx/react/utils/react/forwardRef';
1111

12+
import { RawClickable } from '@lumx/react/utils/react/RawClickable';
1213
import { CLASSNAME as ITEM_CLASSNAME } from './NavigationItem';
1314
import { NavigationContext } from './context';
1415

@@ -52,7 +53,8 @@ export const NavigationSection = forwardRef<NavigationSectionProps, HTMLLIElemen
5253
)}
5354
ref={ref}
5455
>
55-
<button
56+
<RawClickable<'button'>
57+
as="button"
5658
{...forwardedProps}
5759
aria-controls={sectionId}
5860
aria-expanded={isOpen}
@@ -62,7 +64,6 @@ export const NavigationSection = forwardRef<NavigationSectionProps, HTMLLIElemen
6264
setIsOpen(!isOpen);
6365
event.stopPropagation();
6466
}}
65-
type="button"
6667
>
6768
{icon ? <Icon className={`${ITEM_CLASSNAME}__icon`} icon={icon} size={Size.xs} /> : null}
6869

@@ -73,7 +74,7 @@ export const NavigationSection = forwardRef<NavigationSectionProps, HTMLLIElemen
7374
className={classNames(`${ITEM_CLASSNAME}__icon`, `${CLASSNAME}__chevron`)}
7475
icon={isOpen ? mdiChevronUp : mdiChevronDown}
7576
/>
76-
</button>
77+
</RawClickable>
7778
{isOpen &&
7879
(isDropdown ? (
7980
<Popover

0 commit comments

Comments
 (0)