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

Forward ref support for TableRow, Tabs, ToggleButton & ToggleButtonGroup #861

Merged
merged 9 commits into from
Nov 29, 2021
Merged
Show file tree
Hide file tree
Changes from 5 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
6 changes: 6 additions & 0 deletions .changeset/nine-comics-work.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@aws-amplify/ui-react": patch
"@aws-amplify/ui": patch
---

Forward ref support for TableRow, Tabs, ToggleButton & ToggleButtonGroup
15 changes: 9 additions & 6 deletions packages/react/src/primitives/Table/TableRow.tsx
Original file line number Diff line number Diff line change
@@ -1,21 +1,24 @@
import classNames from 'classnames';
import * as React from 'react';

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

export const TableRow: Primitive<TableRowProps, 'tr'> = ({
children,
className,
...rest
}) => (
const TableRowPrimitive: PrimitiveWithForwardRef<TableRowProps, 'tr'> = (
{ children, className, ...rest },
ref
) => (
<View
as="tr"
className={classNames(ComponentClassNames.TableRow, className)}
ref={ref}
{...rest}
>
{children}
</View>
);

export const TableRow = React.forwardRef(TableRowPrimitive);

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

import {
Table,
Expand Down Expand Up @@ -154,4 +155,14 @@ describe('Table primitive', () => {
expect($table).toHaveAttribute('data-variation', variation);
});
});

describe('Forward ref: ', () => {
it('should forward ref to TableRow DOM element', async () => {
const ref = React.createRef<HTMLTableRowElement>();
render(<TableRow ref={ref} />);

await screen.findByRole('row');
expect(ref.current.nodeName).toBe('TR');
});
});
});
91 changes: 48 additions & 43 deletions packages/react/src/primitives/Tabs/Tabs.tsx
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
import * as React from 'react';
import classNames from 'classnames';
import {
Root,
List,
Trigger as RadixTab,
Content as Panel,
} from '@radix-ui/react-tabs';
import { TabsProps, TabItemProps } from '../types';
import * as React from 'react';

import { Flex } from '../Flex';
import { View } from '../View';
import { TabsProps, TabItemProps, PrimitiveWithForwardRef } from '../types';
import { ComponentClassNames } from '../shared/constants';
import classNames from 'classnames';
import { convertStylePropsToStyleObj, prefixer } from '../shared/styleUtils';
import { Primitive } from '../types';

const isTabsType = (child: any): child is React.Component<TabItemProps> => {
return (
Expand All @@ -21,29 +21,26 @@ const isTabsType = (child: any): child is React.Component<TabItemProps> => {
);
};

export const Tabs: Primitive<TabsProps, typeof Flex> = ({
alignContent,
alignItems,
ariaLabel,
children,
className,
defaultIndex = 0,
currentIndex,
onChange,
indicatorPosition,
direction,
gap = '0',
spacing,
justifyContent,
wrap,
...rest
}: TabsProps) => {
const TabsPrimitive: PrimitiveWithForwardRef<TabsProps, typeof Flex> = (
{
ariaLabel,
children,
className,
defaultIndex = 0,
currentIndex,
onChange,
indicatorPosition,
spacing,
...rest
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning this up

}: TabsProps,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
}: TabsProps,
}

ref
) => {
const tabs = React.Children.map(children, (child) => {
if (!isTabsType(child)) {
console.warn(
'Amplify UI: <Tabs> component only accepts <TabItem> as children.'
);
return null;
return {};
Copy link
Contributor Author

@zchenwei zchenwei Nov 23, 2021

Choose a reason for hiding this comment

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

[..., null, ...] will throw error on mapping operation. Cannot deconstruct null or reference to a prop

}

return child.props;
Expand All @@ -58,33 +55,26 @@ export const Tabs: Primitive<TabsProps, typeof Flex> = ({
value: currentIndex != null ? currentIndex.toString() : undefined,
onValueChange: onChange,
};

return (
<Root {...rootProps}>
<List aria-label={ariaLabel}>
<Flex
alignContent={alignContent}
alignItems={alignItems}
className={classNames(ComponentClassNames.Tabs, className)}
direction={direction}
gap={gap}
justifyContent={justifyContent}
wrap={wrap}
data-indicator-position={indicatorPosition}
ref={ref}
{...rest}
>
{tabs.map(({ className, isDisabled, title, ...rest }, index) => (
<RadixTab
className={classNames(ComponentClassNames.TabItems, className)}
data-spacing={spacing}
disabled={isDisabled}
key={index}
style={prefixer(convertStylePropsToStyleObj(rest))}
value={`${index}`}
>
{title}
</RadixTab>
))}
{React.Children.map(children, (child, index) => {
if (!isTabsType(child)) {
return null;
}

return React.cloneElement(child, {
Copy link
Contributor Author

@zchenwei zchenwei Nov 23, 2021

Choose a reason for hiding this comment

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

By cloning, we are able to clone the ref passed along. I cannot find a way to reference ref directly from child

['data-spacing']: spacing,
key: index,
value: `${index}`,
});
})}
</Flex>
</List>
{tabs.map((tab, index) => (
Expand All @@ -96,7 +86,22 @@ export const Tabs: Primitive<TabsProps, typeof Flex> = ({
);
};

export const TabItem: React.FC<TabItemProps> = () => <></>;
const TabItemPrimitvie: PrimitiveWithForwardRef<TabItemProps, 'div'> = (
zchenwei marked this conversation as resolved.
Show resolved Hide resolved
{ className, title, ...rest },
ref
) => (
<View
as={RadixTab}
Copy link
Contributor

Choose a reason for hiding this comment

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

Woah, so cool that this works!

className={classNames(ComponentClassNames.TabItems, className)}
ref={ref}
{...rest}
>
{title}
</View>
);

export const Tabs = React.forwardRef(TabsPrimitive);
export const TabItem = React.forwardRef(TabItemPrimitvie);

Tabs.displayName = 'Tabs';
TabItem.displayName = 'TabItem';
32 changes: 30 additions & 2 deletions packages/react/src/primitives/Tabs/__tests__/Tabs.test.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { render, screen } from '@testing-library/react';
import * as React from 'react';

import { Tabs, TabItem } from '../Tabs';
import { Text } from '../../Text';
import { ComponentClassNames } from '../../shared';
import { ComponentPropsToStylePropsMap } from '../../types';
import kebabCase from 'lodash/kebabCase';

describe('Tabs: ', () => {
it('can render custom classnames', async () => {
Expand All @@ -29,6 +29,19 @@ describe('Tabs: ', () => {
expect(tabs.dataset['demo']).toBe('true');
});

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

render(
<Tabs ref={ref} testId="tabsId">
<TabItem title="Tab 1">Tab 1</TabItem>
</Tabs>
);

await screen.findByTestId('tabsId');
expect(ref.current.nodeName).toBe('DIV');
});

describe('TabItem: ', () => {
it('can render custom classnames', async () => {
render(
Expand Down Expand Up @@ -67,5 +80,20 @@ describe('Tabs: ', () => {
const tab = await screen.findByRole('tab');
expect(tab).toHaveAttribute('aria-disabled');
});

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

render(
<Tabs>
<TabItem ref={ref} title="Tab 1">
Tab 1
</TabItem>
</Tabs>
);

await screen.findByRole('tab');
expect(ref.current.nodeName).toBe('DIV');
});
});
});
38 changes: 24 additions & 14 deletions packages/react/src/primitives/ToggleButton/ToggleButton.tsx
Original file line number Diff line number Diff line change
@@ -1,23 +1,30 @@
import classNames from 'classnames';
import * as React from 'react';

import { useToggleButton } from './useToggleButton';
import { Button } from '../Button';
import { ToggleButtonProps, Primitive } from '../types';
import { ToggleButtonProps, PrimitiveWithForwardRef } from '../types';
import { ComponentClassNames } from '../shared/constants';

export const ToggleButton: Primitive<ToggleButtonProps, typeof Button> = ({
className,
children,
defaultPressed = false,
isDisabled,
isPressed: isPressedProp,
onChange,
onClick,
size,
value,
variation,
...rest
}) => {
const ToggleButtonPrimitive: PrimitiveWithForwardRef<
ToggleButtonProps,
typeof Button
> = (
{
className,
children,
defaultPressed = false,
isDisabled,
isPressed: isPressedProp,
onChange,
onClick,
size,
value,
variation,
...rest
},
ref
) => {
const { isPressed, handleClick } = useToggleButton({
isPressed: isPressedProp,
defaultPressed,
Expand All @@ -31,6 +38,7 @@ export const ToggleButton: Primitive<ToggleButtonProps, typeof Button> = ({
className={classNames(ComponentClassNames.ToggleButton, className)}
isDisabled={isDisabled}
onClick={handleClick}
ref={ref}
size={size}
type="button"
variation={variation}
Expand All @@ -41,4 +49,6 @@ export const ToggleButton: Primitive<ToggleButtonProps, typeof Button> = ({
);
};

export const ToggleButton = React.forwardRef(ToggleButtonPrimitive);

ToggleButton.displayName = 'ToggleButton';
Original file line number Diff line number Diff line change
@@ -1,13 +1,13 @@
import { useState } from 'react';
import { render, screen } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import * as React from 'react';

import { ComponentClassNames } from '../../shared';
import { ToggleButton } from '../ToggleButton';

describe('ToggleButton: ', () => {
const ControlledToggleButton = () => {
const [pressed, setPressed] = useState(false);
const [pressed, setPressed] = React.useState(false);
return (
<ToggleButton isPressed={pressed} onChange={() => setPressed(!pressed)} />
);
Expand Down Expand Up @@ -67,4 +67,12 @@ describe('ToggleButton: ', () => {
userEvent.click(toggleButton);
expect(toggleButton).toHaveAttribute('aria-pressed', 'false');
});

it('should forward ref to DOM element', async () => {
const ref = React.createRef<HTMLButtonElement>();
render(<ToggleButton ref={ref} />);

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