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.

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

Add forwardRef support to Button & View #698

Merged
merged 3 commits into from
Nov 12, 2021
Merged
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
5 changes: 5 additions & 0 deletions .changeset/dirty-keys-eat.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@aws-amplify/ui-react": minor
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should this be major since it's technically a change in behavior?

---

Add forwardRef support to Button & View
72 changes: 41 additions & 31 deletions packages/react/src/primitives/Button/Button.tsx
Original file line number Diff line number Diff line change
@@ -1,37 +1,47 @@
import classNames from 'classnames';
import * as React from 'react';

import { ComponentClassNames } from '../shared/constants';
import { ButtonProps, Primitive } from '../types';
import { ButtonProps, PrimitiveWithForwardRef } from '../types';
import { View } from '../View';

export const Button: Primitive<ButtonProps, 'button'> = ({
className,
children,
isFullWidth = false,
isDisabled,
isLoading,
loadingText = '',
size,
type = 'button',
variation,
...rest
}) => (
<View
as="button"
className={classNames(
ComponentClassNames.Button,
ComponentClassNames.FieldGroupControl,
className
)}
data-fullwidth={isFullWidth}
data-loading={isLoading}
data-size={size}
data-variation={variation}
isDisabled={isDisabled || isLoading}
type={type}
{...rest}
>
{isLoading && loadingText ? <span>{loadingText}</span> : children}
</View>
);
const ButtonInner: PrimitiveWithForwardRef<ButtonProps, 'button'> = (
{
className,
children,
isFullWidth = false,
isDisabled,
isLoading,
loadingText = '',
size,
type = 'button',
variation,
...rest
},
ref
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added ref as second parameter

) => {
return (
<View
ref={ref}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the only change to the returned jsx, setting the ref prop to be forwarded.

as="button"
className={classNames(
ComponentClassNames.Button,
ComponentClassNames.FieldGroupControl,
className
)}
data-fullwidth={isFullWidth}
data-loading={isLoading}
data-size={size}
data-variation={variation}
isDisabled={isDisabled || isLoading}
type={type}
{...rest}
>
{isLoading && loadingText ? <span>{loadingText}</span> : children}
</View>
);
};

export const Button = React.forwardRef(ButtonInner);

Button.displayName = 'Button';
12 changes: 12 additions & 0 deletions packages/react/src/primitives/Button/__tests__/Button.test.tsx
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as React from 'react';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

Expand All @@ -13,6 +14,17 @@ describe('Button test suite', () => {
expect(button).toHaveClass(ComponentClassNames.Button, className);
});

it('should forward ref to button DOM element', async () => {
const ref = React.createRef<HTMLButtonElement>();
const buttonText = 'Hello there';

render(<Button ref={ref}>{buttonText}</Button>);

await screen.findByRole('button');
expect(ref.current.nodeName).toBe('BUTTON');
expect(ref.current.innerHTML).toBe(buttonText);
});

it('should set size and variation props correctly', async () => {
const size = 'large';
const variation = 'primary';
Expand Down
34 changes: 21 additions & 13 deletions packages/react/src/primitives/View/View.tsx
Original file line number Diff line number Diff line change
@@ -1,19 +1,22 @@
import React from 'react';
import { useNonStyleProps, usePropStyles } from '../shared/styleUtils';
import { ElementType, PrimitiveProps, ViewProps } from '../types';
import { ElementType, PrimitivePropsWithRef, ViewProps } from '../types';

export const View = <Element extends ElementType = 'div'>({
as = 'div',
className,
children,
role,
id,
testId,
ariaLabel,
isDisabled,
style,
...rest
}: PrimitiveProps<ViewProps, Element>) => {
const ViewInner = <Element extends ElementType = 'div'>(
{
as = 'div',
className,
children,
role,
id,
testId,
ariaLabel,
isDisabled,
style,
...rest
}: PrimitivePropsWithRef<ViewProps, Element>,
ref?: React.ForwardedRef<HTMLElement>
) => {
const propStyles = usePropStyles(rest, style);
const nonStyleProps = useNonStyleProps(rest);

Expand All @@ -26,9 +29,14 @@ export const View = <Element extends ElementType = 'div'>({
disabled: isDisabled,
id,
role,
ref,
style: propStyles,
...nonStyleProps,
},
children
);
};

export const View = React.forwardRef(ViewInner);

View.displayName = 'View';
18 changes: 16 additions & 2 deletions packages/react/src/primitives/View/__tests__/View.test.tsx
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import { View } from '../View';
import * as React from 'react';

import { kebabCase } from 'lodash';
import { render, screen } from '@testing-library/react';

import { ComponentPropsToStylePropsMap } from '../../types';
import { kebabCase } from 'lodash';
import { View } from '../View';

describe('View: ', () => {
const viewText = 'Hello from inside a view';
Expand All @@ -22,6 +25,17 @@ describe('View: ', () => {
expect(view.nodeName).toBe('DIV');
});

it('should forward ref to DOM element', async () => {
const ref = React.createRef<HTMLDivElement>();
const text = 'Hello there';

render(<View ref={ref}>{text}</View>);

await screen.findByText(text);
expect(ref.current.nodeName).toBe('DIV');
expect(ref.current.innerHTML).toBe(text);
});

it('can render a <button> HTML element', async () => {
render(<View as="button">{viewText}</View>);
const viewButton = await screen.findByRole('button');
Expand Down
35 changes: 33 additions & 2 deletions packages/react/src/primitives/types/view.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,20 @@ type MergeProps<A, B> = A & Omit<B, keyof A>;

export type ElementType = React.FC<any> | keyof JSX.IntrinsicElements;

/**
* Convert string element type to DOMElement Type
* e.g. 'button' => HTMLButtonElement
*/
export type HTMLElementType<Element extends ElementType> =
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hat tip to @hvergara for this type magic

Element extends keyof JSX.IntrinsicElements
? JSX.IntrinsicElements[Element] extends React.DetailedHTMLProps<
Copy link
Contributor

Choose a reason for hiding this comment

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

This could potentially be Element extends keyof JSX.IntrinsicElements ? React.ElementRef<Element> : HTMLDivElement; ? <- https://flow.org/en/docs/react/types/#toc-react-elementref

And can you explain why always return HTMLDivElement for React.FC<any>? 😕

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It might be more accurate to return HTMLElement instead of HTMLDivElement. Thoughts @hvergara?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am asking because we do have SVGELement, though the primitives don't need ref in large part

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, good idea

unknown,
infer HTMLType
>
? HTMLType
: never
: HTMLDivElement;

export type ElementProps<Element extends ElementType> =
Element extends keyof JSX.IntrinsicElements
? JSX.IntrinsicElements[Element]
Expand All @@ -18,15 +32,32 @@ export type PrimitiveProps<
Props extends ViewProps,
Element extends ElementType
> = MergeProps<
Omit<Props, 'as'> & { as?: Element | Props['as'] },
ElementProps<Element>
Omit<Props, 'as'> & {
as?: Element | Props['as'];
},
Omit<ElementProps<Element>, 'ref'> // exclude `ref?: LegacyRef` included in DetailedHTMLProps
Copy link
Contributor Author

@reesscot reesscot Nov 12, 2021

Choose a reason for hiding this comment

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

This Omit is required because JSX.IntrinsicElements[Element] resolves to React.DetailedHTMLProps, which resolves to ClassAttributes, which contains the ref?: LegacyRef<T>. AFAIK, it's invalid to include the ref prop in a functional component, and causes type errors on functional components which use the forwardRef version of View or Button. For example, Text throws a type error when passing props to View because React.LegacyRef and React.Ref are not compatible.

Copy link
Contributor

Choose a reason for hiding this comment

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

🙌🏽

>;

export type PrimitivePropsWithRef<
Props extends ViewProps,
Element extends ElementType
> = PrimitiveProps<Props, Element> & {
ref?: React.Ref<HTMLElementType<Element>>;
};

export type Primitive<
Props extends ViewProps,
Element extends ElementType
> = React.FC<PrimitiveProps<Props, Element>>;

export type PrimitiveWithForwardRef<
Props extends ViewProps,
Element extends ElementType
> = React.ForwardRefRenderFunction<
HTMLElementType<Element>,
PrimitivePropsWithRef<Props, Element>
>;

export interface ViewProps
extends BaseComponentProps,
BaseStyleProps,
Expand Down