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

Jump to bottom

fix(ui-react): Whitespace in Tab IDs #5378

Merged
merged 6 commits into from
Aug 6, 2024
Merged

fix(ui-react): Whitespace in Tab IDs #5378

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
240f5b1
fix: whitespace in tabs, validate value & defaultValue types
axelEandrews Jul 9, 2024
1cdb52a
chore: added changeset
axelEandrews Jul 9, 2024
f3701a8
fix: creates const idValue to avoid changing inputted values
axelEandrews Jul 11, 2024
14e72ac
fix: removed unused import, type-checked before .replace
axelEandrews Jul 11, 2024
3749098
fix: called functions in test
axelEandrews Jul 19, 2024
7a9bcdd
fix: updated ComposedTabsExample.tsx
axelEandrews Aug 1, 2024
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/tall-olives-beam.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@aws-amplify/ui-react': patch
---

fixes invalid tab IDs
4 changes: 2 additions & 2 deletions docs/src/pages/[platform]/components/tabs/examples/ComposedTabsExample.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,9 @@ export const ComposedTabsExample = () => {
</Tabs.Item>
</Tabs.List>
<Tabs.Panel value="Tab 1">Tab 1 content</Tabs.Panel>
<Tabs.Panel value="Tab 2">Tab 1 content</Tabs.Panel>
<Tabs.Panel value="Tab 2">Tab 2 content</Tabs.Panel>
<Tabs.Panel value="Tab 3" isDisabled>
Tab 1 content
Tab 3 content
</Tabs.Panel>
</Tabs.Container>
);
Expand Down
5 changes: 4 additions & 1 deletion packages/react/src/primitives/Tabs/TabsContainer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { primitiveWithForwardRef } from '../utils/primitiveWithForwardRef';
import { BaseTabsProps, TabsProps } from './types';
import { View } from '../View';
import { TabsContext } from './TabsContext';
import { useStableId } from '../utils/useStableId';

const TabsContainerPrimitive: Primitive<TabsProps, 'div'> = (
{
Expand All @@ -20,6 +21,7 @@ const TabsContainerPrimitive: Primitive<TabsProps, 'div'> = (
}: BaseTabsProps,
ref
) => {
const groupId = useStableId(); // groupId is used to ensure uniqueness between Tab Groups in IDs
const isControlled = controlledValue !== undefined;
const [localValue, setLocalValue] = React.useState(() =>
isControlled ? controlledValue : defaultValue
Expand All @@ -44,8 +46,9 @@ const TabsContainerPrimitive: Primitive<TabsProps, 'div'> = (
activeTab,
isLazy,
setActiveTab,
groupId,
};
}, [activeTab, setActiveTab, isLazy]);
}, [activeTab, setActiveTab, isLazy, groupId]);

return (
<TabsContext.Provider value={_value}>
Expand Down
2 changes: 2 additions & 0 deletions packages/react/src/primitives/Tabs/TabsContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,12 @@ import * as React from 'react';
export interface TabsContextInterface {
activeTab: string;
isLazy?: boolean;
groupId: string;
setActiveTab: (tab: string) => void;
}

export const TabsContext = React.createContext<TabsContextInterface>({
groupId: '',
activeTab: '',
setActiveTab: () => {},
});
12 changes: 8 additions & 4 deletions packages/react/src/primitives/Tabs/TabsItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,28 +12,32 @@ import { View } from '../View';
import { primitiveWithForwardRef } from '../utils/primitiveWithForwardRef';
import { BaseTabsItemProps, TabsItemProps } from './types';
import { TabsContext } from './TabsContext';
import { WHITESPACE_VALUE } from './constants';

const TabsItemPrimitive: Primitive<TabsItemProps, 'button'> = (
{ className, value, children, onClick, as = 'button', role = 'tab', ...rest },
ref
) => {
const { activeTab, setActiveTab } = React.useContext(TabsContext);
const { activeTab, setActiveTab, groupId } = React.useContext(TabsContext);
let idValue = value;
if (typeof idValue === 'string') {
idValue = idValue.replace(' ', WHITESPACE_VALUE);
}
const isActive = activeTab === value;
const handleOnClick = (e: React.MouseEvent<HTMLButtonElement>) => {
if (isTypedFunction(onClick)) {
onClick?.(e);
}
setActiveTab(value);
};

return (
<View
{...rest}
role={role}
as={as}
id={`${value}-tab`}
id={`${groupId}-tab-${idValue}`}
aria-selected={isActive}
aria-controls={`${value}-panel`}
aria-controls={`${groupId}-panel-${idValue}`}
Comment on lines +38 to +40
Copy link
Member

Choose a reason for hiding this comment

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

If idValue is undefined then the values used on lines 38 and 40 will not be unique

Copy link
Contributor Author

Choose a reason for hiding this comment

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

value for TabsPanel and TabsItem is required to be a string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the testing I've done this change does not seem to be breaking. At this stage of the Tabs composition value is required to be defined, so creating idValue based on it does not seem to introduce any errors. Because value itself is not changed, as long as defaultValue matches the intended target Tab's value the controllability and default functionality seems to work as intended. Tested by changing value and defaultValue for the examples to include whitespace.

tabIndex={!isActive ? -1 : undefined}
className={classNames(
ComponentClassName.TabsItem,
Expand Down
12 changes: 9 additions & 3 deletions packages/react/src/primitives/Tabs/TabsPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,21 +8,27 @@ import { View } from '../View';
import { BaseTabsPanelProps, TabsPanelProps } from './types';
import { primitiveWithForwardRef } from '../utils/primitiveWithForwardRef';
import { TabsContext } from './TabsContext';
import { WHITESPACE_VALUE } from './constants';

const TabPanelPrimitive: Primitive<TabsPanelProps, 'div'> = (
{ className, value, children, role = 'tabpanel', ...rest },
ref
) => {
const { activeTab, isLazy } = React.useContext(TabsContext);
const { activeTab, isLazy, groupId } = React.useContext(TabsContext);

if (isLazy && activeTab !== value) return null;

let idValue = value;
if (typeof idValue === 'string') {
idValue = idValue.replace(' ', WHITESPACE_VALUE);
}

return (
<View
{...rest}
role={role}
id={`${value}-panel`}
aria-labelledby={`${value}-tab`}
id={`${groupId}-panel-${idValue}`}
aria-labelledby={`${groupId}-tab-${idValue}`}
className={classNames(
ComponentClassName.TabsPanel,
classNameModifierByFlag(
Expand Down
55 changes: 55 additions & 0 deletions packages/react/src/primitives/Tabs/__tests__/Tabs.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,61 @@ describe('Tabs', () => {
expect(tabs[1]).toHaveAttribute('aria-selected', 'true');
});

it('creates unique IDs for two tabs with same value in different tab groups"', async () => {
render(
<Tabs.Container>
<Tabs.List>
<Tabs.Item value="Tab 1">Tab 1</Tabs.Item>
</Tabs.List>
</Tabs.Container>
);
render(
<Tabs.Container>
<Tabs.List>
<Tabs.Item value="Tab 1">Tab 1</Tabs.Item>
</Tabs.List>
</Tabs.Container>
);
const tabs = await screen.findAllByRole('tab');
expect(tabs[0].id === tabs[1].id).toBeFalsy();
});

it('creates unique ids for each tab with a unique value', async () => {
render(
<Tabs.Container testId="tabsTest">
<Tabs.List>
<Tabs.Item value="1">Tab 1</Tabs.Item>
<Tabs.Item value="2">Tab 2</Tabs.Item>
<Tabs.Item value="3">Tab 3</Tabs.Item>
</Tabs.List>
<Tabs.Panel value="1">Tab 1</Tabs.Panel>
<Tabs.Panel value="2">Tab 2</Tabs.Panel>
<Tabs.Panel value="3">Tab 3</Tabs.Panel>
</Tabs.Container>
);
const tabs = await screen.findAllByRole('tab');
expect(tabs[0].id === tabs[1].id).toBeFalsy();
expect(tabs[0].id === tabs[2].id).toBeFalsy();
});

it('creates the same ids tabs with the same value in the same group', async () => {
render(
<Tabs.Container testId="tabsTest">
<Tabs.List>
<Tabs.Item value="1">Tab 1</Tabs.Item>
<Tabs.Item value="1">Tab 2</Tabs.Item>
<Tabs.Item value="1">Tab 3</Tabs.Item>
</Tabs.List>
<Tabs.Panel value="1">Tab 1</Tabs.Panel>
<Tabs.Panel value="1">Tab 2</Tabs.Panel>
<Tabs.Panel value="1">Tab 3</Tabs.Panel>
</Tabs.Container>
);
const tabs = await screen.findAllByRole('tab');
expect(tabs[0].id === tabs[1].id).toBeTruthy();
expect(tabs[0].id === tabs[2].id).toBeTruthy();
});

describe('TabItem', () => {
it('can render custom classnames', async () => {
render(
Expand Down
2 changes: 2 additions & 0 deletions packages/react/src/primitives/Tabs/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
/* WHITESPACE_VALUE is used to fill whitespace present in user-inputed `value` when creating id for TabsItem and TabsPanel */
export const WHITESPACE_VALUE = '-';
Loading