Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UNSAFE classname test #1826

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+155 −68
Open

UNSAFE classname test #1826

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7596c81
UNSAFE classname test
pavelklibani Dec 18, 2024
7f8766a
fixup! UNSAFE classname test
pavelklibani Jan 23, 2025
4da9753
fixup! fixup! UNSAFE classname test
pavelklibani Jan 23, 2025
3d279e0
fixup! fixup! fixup! UNSAFE classname test
pavelklibani Jan 30, 2025
5d066d4
fixup! fixup! fixup! fixup! UNSAFE classname test
pavelklibani Jan 30, 2025
0a1ec14
fixup! fixup! fixup! fixup! fixup! UNSAFE classname test
pavelklibani Jan 30, 2025
96b1267
fixup! fixup! fixup! fixup! fixup! fixup! UNSAFE classname test
pavelklibani Jan 30, 2025
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
3 changes: 2 additions & 1 deletion examples/next-with-app-router/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@
"@lmc-eu/spirit-web-react": "workspace:^",
"next": "14.2.23",
"react": "^18",
"react-dom": "^18"
"react-dom": "^18",
"sass": "^1.83.0"
},
"devDependencies": {
"@next/eslint-plugin-next": "14.2.23",
Expand Down
4 changes: 3 additions & 1 deletion packages/web-react/src/components/Button/Button.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const _Button = <T extends ElementType = 'button', C = void, S = void>(

const { buttonProps } = useButtonAriaProps(restProps);
const { classProps, props: modifiedProps } = useButtonStyleProps(restProps);
const { styleProps, props: otherProps } = useStyleProps(modifiedProps);
const { styleProps, props: otherProps } = useStyleProps({ ElementTag, ...modifiedProps });

return (
<ElementTag
Expand All @@ -52,4 +52,6 @@ const _Button = <T extends ElementType = 'button', C = void, S = void>(

const Button = forwardRef<HTMLButtonElement, SpiritButtonProps<ElementType>>(_Button);

Button.spiritComponent = 'Button';

export default Button;
24 changes: 10 additions & 14 deletions packages/web-react/src/components/Dropdown/DropdownTrigger.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
'use client';

import classNames from 'classnames';
import React, { ElementType } from 'react';
import { useStyleProps } from '../../hooks';
import { DropdownTriggerProps } from '../../types';
Expand All @@ -14,26 +13,23 @@ const defaultProps = {

const DropdownTrigger = <T extends ElementType = 'button'>(props: DropdownTriggerProps<T>) => {
const propsWithDefaults = { ...defaultProps, ...props };
const { elementType = 'button', children, ...rest } = propsWithDefaults;
const { elementType: ElementTag = 'button', children, ...rest } = propsWithDefaults;
const { id, isOpen, onToggle, fullWidthMode, triggerRef } = useDropdownContext();
const Component = elementType;
const { classProps, props: modifiedProps } = useDropdownStyleProps({ isOpen, ...rest });
const { styleProps, props: otherProps } = useStyleProps(modifiedProps);
const { styleProps: triggerStyleProps, props: transferProps } = useStyleProps({
ElementTag,
transferClassName: classProps.trigger,
...modifiedProps,
});
const { triggerProps } = useDropdownAriaProps({ id, isOpen, toggleHandler: onToggle, fullWidthMode });

return (
<Component
{...rest} // ⚠️ This is maybe a bug, when component is pass via `elementType` prop, the rest props are passed to the component
{...otherProps}
{...triggerProps}
id={id}
ref={triggerRef}
className={classNames(classProps.trigger, styleProps.className)}
style={styleProps.style}
>
<ElementTag {...transferProps} {...triggerProps} {...triggerStyleProps} id={id} ref={triggerRef}>
{typeof children === 'function' ? children({ isOpen }) : children}
</Component>
</ElementTag>
);
};

DropdownTrigger.spiritComponent = 'DropdownTrigger';

export default DropdownTrigger;
8 changes: 8 additions & 0 deletions packages/web-react/src/components/Dropdown/__tests__/DropdownTrigger.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,16 @@ import DropdownTrigger from '../DropdownTrigger';
jest.mock('../../../hooks/useIcon');

describe('DropdownTrigger', () => {
// pass style props to the default trigger
stylePropsTest((props) => <DropdownTrigger {...props} />);

// pass style props to the custom trigger
stylePropsTest((props) => <DropdownTrigger elementType={Button} {...props} />);

// pass rest props to the default trigger
restPropsTest((props) => <DropdownTrigger {...props} />, 'button');

// pass rest props to the custom trigger
restPropsTest((props) => <DropdownTrigger elementType={Button} {...props} />, 'button');

it('should have Button elementType', () => {
Expand Down
5 changes: 3 additions & 2 deletions packages/web-react/src/components/Tooltip/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const [open, setOpen] = React.useState(false);
</Tooltip>;
```

### Trigger
### TooltipTrigger

You can choose whether you want to open the tooltip on `click` and/or `hover`.
By default, both options are active, e.g., `trigger={['click', 'hover']}`.
Expand Down Expand Up @@ -60,6 +60,7 @@ const [open, setOpen] = React.useState(false);
| Attribute | Type | Default | Required | Description |
| ------------------------------- | ----------------------------------------------------------------- | -------------------- | -------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `children` | `ReactNode` | — | ✓ | Tooltip children's nodes - `TooltipTrigger` and `TooltipPopover` |
| `elementType` | `ElementType` | "button" | ✕ | Type of element used as trigger |
| `enableFlipping` | `bool` | true | ✕ | Enables [flipping][floating-ui-flip] of the element’s placement when it starts to overflow its boundary area. For example `top` can be flipped to `bottom`. |
| `enableFlippingCrossAxis` | `bool` | true | ✕ | Enables flipping on the [cross axis][floating-ui-flip-cross-axis], the axis perpendicular to main axis. For example `top-end` can be flipped to the `top-start`. |
| `enableShifting` | `bool` | true | ✕ | Enables [shifting][floating-ui-shift] of the element to keep it inside the boundary area by adjusting its position. |
Expand All @@ -68,11 +69,11 @@ const [open, setOpen] = React.useState(false);
| `flipFallbackPlacements` | `string` | - | ✕ | This describes a list of [explicit placements][floating-ui-flip-fallback-placements] to try if the initial placement doesn’t fit on the axes in which overflow is checked. For example you can set `"top, right, bottom"` |
| `id` | `string` | - | ✓ | Tooltip id |
| `isDismissible` | `bool` | false | ✕ | Make tooltip dismissible |
| `isFocusableOnHover` | `bool` | false | ✕ | Allows you to mouse over a tooltip without closing it. We suggest turning off the `click` trigger if you use this feature. |
| `isOpen` | `bool` | - | ✓ | Open state |
| `onToggle` | `() => void` | - | ✓ | Function for toggle open state of dropdown |
| `placement` | [Placement Dictionary][dictionary-placement] | "bottom" | ✕ | Placement of tooltip |
| `positionStrategy` | \[`absolute` \| `fixed`] ([Strategy type][use-floating-strategy]) | "absolute" | ✕ | This is the type of CSS position property to use. |
| `isFocusableOnHover` | `bool` | false | ✕ | Allows you to mouse over a tooltip without closing it. We suggest turning off the `click` trigger if you use this feature. |
| `trigger` | \[`click` \| `hover` \| `manual`] | \["click", "hover" ] | ✕ | How tooltip is triggered: `click`, `hover`, `manual`. You may pass multiple triggers. If you pass `manual`, there will be no toggle functionality and you should provide your own toggle solution. |

On top of the API options, the components accept [additional attributes][readme-additional-attributes].
Expand Down
23 changes: 9 additions & 14 deletions packages/web-react/src/components/Tooltip/TooltipTrigger.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,29 @@
'use client';

import React, { ElementType, ReactNode } from 'react';
import React from 'react';
import { useStyleProps } from '../../hooks';
import { StyleProps, TransferProps } from '../../types';
import { TooltipTriggerProps } from '../../types';
import { useTooltipContext } from './TooltipContext';

interface TooltipTriggerProps extends StyleProps, TransferProps {
elementType?: ElementType | string;
children?: string | ReactNode | ((props: { isOpen: boolean }) => ReactNode);
}

const defaultProps: TooltipTriggerProps = {
const defaultProps: Partial<TooltipTriggerProps> = {
elementType: 'button',
children: null,
};

const TooltipTrigger = (props: TooltipTriggerProps) => {
const propsWithDefaults = { ...defaultProps, ...props };
const { elementType = 'button', children, ...rest } = propsWithDefaults;
const { elementType: ElementTag = 'button', children, ...rest } = propsWithDefaults;
const { id, isOpen, triggerRef, getReferenceProps } = useTooltipContext();

const Component = elementType;

const { styleProps: triggerStyleProps, props: transferProps } = useStyleProps(rest);
const { styleProps: triggerStyleProps, props: transferProps } = useStyleProps({ ElementTag, ...rest });

return (
<Component {...transferProps} {...triggerStyleProps} id={id} ref={triggerRef} {...getReferenceProps()}>
<ElementTag {...transferProps} {...triggerStyleProps} id={id} ref={triggerRef} {...getReferenceProps()}>
{typeof children === 'function' ? children({ isOpen }) : children}
</Component>
</ElementTag>
);
};

TooltipTrigger.spiritComponent = 'TooltipTrigger';

export default TooltipTrigger;
9 changes: 9 additions & 0 deletions packages/web-react/src/hooks/__tests__/styleProps.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { renderHook } from '@testing-library/react';
import { Button } from '../../components/Button';
import { StyleProps } from '../../types';
import { useStyleProps } from '../styleProps';

Expand All @@ -15,6 +16,10 @@ describe('styleProps', () => {
{ className: undefined, style: { 'vertical-align': 'center' } },
],
[{ role: 'button' }, { className: undefined, style: undefined }],
[
{ ElementTag: Button, UNSAFE_className: 'test-class' },
{ UNSAFE_className: 'test-class', style: undefined },
],
])('should use UNSAFE_style and UNSAFE_className props', (input, expected) => {
const { result } = renderHook(() => useStyleProps(input as StyleProps));

Expand Down Expand Up @@ -125,6 +130,10 @@ describe('styleProps', () => {
{ margin: 'space-100', UNSAFE_className: 'm-500' },
{ className: 'm-500 m-100', style: undefined },
],
[
{ ElementTag: Button, margin: 'space-100', UNSAFE_className: 'm-500' },
{ UNSAFE_className: 'm-500 m-100', style: undefined },
],
])('should return correct combination of class and style', (input, expected) => {
const { result } = renderHook(() => useStyleProps(input as StyleProps));

Expand Down
85 changes: 52 additions & 33 deletions packages/web-react/src/hooks/styleProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,15 @@ import ClassNamePrefixContext from '../context/ClassNamePrefixContext';
import { StyleProps } from '../types';
import { useStyleUtilities } from './useStyleUtilities';

export type UnsafeStylePropsResult = {
UNSAFE_className?: string;
UNSAFE_style?: CSSProperties;
className?: string;
style?: CSSProperties;
};

export type StylePropsResult = {
styleProps: HTMLAttributes<HTMLElement>;
styleProps: HTMLAttributes<HTMLElement> | UnsafeStylePropsResult;
props: HTMLAttributes<HTMLElement>;
};

Expand All @@ -15,46 +22,58 @@ export function useStyleProps<T extends StyleProps>(
additionalUtilities?: Record<string, string>,
): StylePropsResult {
const classNamePrefix = useContext(ClassNamePrefixContext);
const { UNSAFE_className, UNSAFE_style, ...otherProps } = props;
const { UNSAFE_className, UNSAFE_style, ElementTag, transferClassName, ...otherProps } = props;
const { styleUtilities, props: modifiedProps } = useStyleUtilities(otherProps, classNamePrefix, additionalUtilities);

const style: CSSProperties = { ...UNSAFE_style };

// Want to check if className prop exists, but not to define it in StyleProps type
// @ts-expect-error Property 'className' does not exist on type 'Omit<T, "UNSAFE_className" | "UNSAFE_style">'.
if (modifiedProps.className) {
warning(
false,
'The className prop is unsafe and is unsupported in Spirit Web React. ' +
'Please use style props with Spirit Design Tokens, or UNSAFE_className if you absolutely must do something custom. ' +
'Note that this may break in future versions due to DOM structure changes.',
);

// @ts-expect-error same as above, let me live my life
delete modifiedProps.className;
}
if (typeof ElementTag === 'string' || !ElementTag?.spiritComponent) {
// Want to check if className prop exists, but not to define it in StyleProps type
// @ts-expect-error Property 'className' does not exist on type 'Omit<T, "UNSAFE_className" | "UNSAFE_style">'.
if (modifiedProps.className) {
warning(
false,
'The className prop is unsafe and is unsupported in Spirit Web React. ' +
'Please use style props with Spirit Design Tokens, or UNSAFE_className if you absolutely must do something custom. ' +
'Note that this may break in future versions due to DOM structure changes.',
);

// Want to check if style prop exists, but not to define it in StyleProps type
// @ts-expect-error Property 'style' does not exist on type 'Omit<T, "UNSAFE_className" | "UNSAFE_style">'.
if (modifiedProps.style) {
warning(
false,
'The style prop is unsafe and is unsupported in Spirit Web React. ' +
'Please use style props with Spirit Design Tokens, or UNSAFE_style if you absolutely must do something custom. ' +
'Note that this may break in future versions due to DOM structure changes.',
);

// @ts-expect-error same as above, let me live my life
delete modifiedProps.style;
}
// @ts-expect-error same as above, let me live my life
delete modifiedProps.className;
}

const styleProps = {
style: Object.keys(style).length > 0 ? style : undefined,
className: classNames(UNSAFE_className, ...styleUtilities) || undefined,
};
// Want to check if style prop exists, but not to define it in StyleProps type
// @ts-expect-error Property 'style' does not exist on type 'Omit<T, "UNSAFE_className" | "UNSAFE_style">'.
if (modifiedProps.style) {
warning(
false,
'The style prop is unsafe and is unsupported in Spirit Web React. ' +
'Please use style props with Spirit Design Tokens, or UNSAFE_style if you absolutely must do something custom. ' +
'Note that this may break in future versions due to DOM structure changes.',
);

// @ts-expect-error same as above, let me live my life
delete modifiedProps.style;
}

const styleProps = {
style: Object.keys(style).length > 0 ? style : undefined,
className: classNames(UNSAFE_className, ...styleUtilities, transferClassName) || undefined,
};

return {
styleProps,
props: { ...(modifiedProps as HTMLAttributes<HTMLElement>) },
};
}

return {
styleProps,
styleProps: {
...(UNSAFE_style !== undefined && { UNSAFE_style }),
...((UNSAFE_className !== undefined || styleUtilities !== undefined) && {
UNSAFE_className: classNames(UNSAFE_className, ...styleUtilities, transferClassName),
}),
},
props: modifiedProps as HTMLAttributes<HTMLElement>,
};
}
19 changes: 19 additions & 0 deletions packages/web-react/src/types/shared/NamedExoticComponent.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
/* eslint-disable @typescript-eslint/ban-types */

import type { ExoticComponent, FC, StaticLifecycle } from 'react';

declare global {
namespace React {
interface NamedExoticComponent<P = {}> extends ExoticComponent<P> {
pavelklibani marked this conversation as resolved.
Show resolved Hide resolved
spiritComponent?: string;
}

interface FunctionComponent<P = {}> extends FC<P> {
spiritComponent?: string;
}

interface ComponentClass<P = {}, S = {}> extends StaticLifecycle<P, S> {
spiritComponent?: string;
}
}
}
6 changes: 5 additions & 1 deletion packages/web-react/src/types/shared/style.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { CSSProperties } from 'react';
import { CSSProperties, ElementType } from 'react';
import { SpacingStyleProp } from '../../constants';
import { BreakpointToken, SpaceToken } from './tokens';

Expand All @@ -20,7 +20,11 @@ export interface SpacingCSSProperties extends CSSProperties {
[index: `--${string}`]: string | undefined | number;
}

export type ElementTypeProp = string | ElementType;

export interface StyleProps extends SpacingProps {
ElementTag?: ElementTypeProp;
transferClassName?: string;
// For backward compatibility!
/** Sets the CSS [className](https://developer.mozilla.org/en-US/docs/Web/API/Element/className) for the element. Only use as a **last resort**. Use style props instead. */
UNSAFE_className?: string;
Expand Down
Loading
Loading