Skip to content

chore(a11yaudit): remove unneeded nav/aria-label from Popover #7745

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

Merged
merged 1 commit into from
Jun 18, 2024
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
6 changes: 1 addition & 5 deletions src/components/GenSwitcher/GenSwitcher.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,7 @@ export const GenSwitcher = ({ isGen1, testId }: GenSwitcherProps) => {
{isGen1 ? 'Gen 1' : 'Gen 2'}
<VisuallyHidden>Open Amplify generation navigation</VisuallyHidden>
</Popover.Trigger>
<Popover.List
ariaLabel="Amplify generation navigation"
anchor="left"
className="gen-switcher__list"
>
<Popover.List anchor="left" className="gen-switcher__list">
<Popover.ListItem href={`/${currentPlatform}`} current={!isGen1}>
Gen 2 {isGen1 ? '' : <IconCheck className="gen-switcher__check" />}
</Popover.ListItem>
Expand Down
6 changes: 4 additions & 2 deletions src/components/GetStartedPopover/GetStartedPopover.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,13 @@ export type GetStartedLinksType = {
type GetStartedPopoverType = {
platform: Platform | typeof DEFAULT_PLATFORM;
getStartedLinks: GetStartedLinksType[];
testId?: string;
};

export const GetStartedPopover = ({
platform,
getStartedLinks
getStartedLinks,
testId
}: GetStartedPopoverType) => {
const isGen1Page = useIsGen1Page();

Expand All @@ -47,7 +49,7 @@ export const GetStartedPopover = ({
Toggle getting started guides navigation
</VisuallyHidden>
</Popover.Trigger>
<Popover.List ariaLabel="Getting started guides for other platforms">
<Popover.List testId={testId ? `${testId}-popoverList` : ''}>
{getStartedLinks.map((link, index) => {
return (
<Popover.ListItem
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ jest.mock('next/router', () => routerMock);

describe('GetStartedPopover', () => {
const getStartedHref = '/[platform]/start/quickstart/';
const testId = 'getStartedTestId';
const popoverTestId = `${testId}-popoverList`;
const getStartedLinks = [
{
title: 'React',
Expand Down Expand Up @@ -112,7 +114,11 @@ describe('GetStartedPopover', () => {
];

const component = (
<GetStartedPopover platform={'react'} getStartedLinks={getStartedLinks} />
<GetStartedPopover
testId={testId}
platform={'react'}
getStartedLinks={getStartedLinks}
/>
);

const componentWithGeneratedLinks = (
Expand Down Expand Up @@ -149,9 +155,7 @@ describe('GetStartedPopover', () => {
const button = await screen.findByRole('button', {
name: 'Toggle getting started guides navigation'
});
const dropdown = await screen.findByRole('navigation', {
name: 'Getting started guides for other platforms'
});
const dropdown = screen.getByTestId(popoverTestId);

expect(dropdown.classList).not.toContain('popover--expanded');
userEvent.click(button);
Expand Down Expand Up @@ -180,9 +184,7 @@ describe('GetStartedPopover', () => {
const button = await screen.findByRole('button', {
name: 'Toggle getting started guides navigation'
});
const dropdown = await screen.findByRole('navigation', {
name: 'Getting started guides for other platforms'
});
const dropdown = screen.getByTestId(popoverTestId);

userEvent.click(button);
expect(dropdown.classList).toContain('popover--expanded');
Expand All @@ -195,9 +197,7 @@ describe('GetStartedPopover', () => {
const button = await screen.findByRole('button', {
name: 'Toggle getting started guides navigation'
});
const dropdown = await screen.findByRole('navigation', {
name: 'Getting started guides for other platforms'
});
const dropdown = screen.getByTestId(popoverTestId);
const platformOptions =
document.getElementsByClassName('popover-list__link');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,15 @@ const flatDirectoryMock = {
jest.mock('@/directory/flatDirectory.json', () => flatDirectoryMock);

describe('PlatformNavigator', () => {
const testId = 'platformNavTestId';
const popoverTestId = `${testId}-popoverList`;

const component = (
<PlatformNavigator currentPlatform={'react'} isGen1={false} />
<PlatformNavigator
testId={testId}
currentPlatform={'react'}
isGen1={false}
/>
);

it('should render the PlatformNavigator component', async () => {
Expand Down Expand Up @@ -71,7 +78,7 @@ describe('PlatformNavigator', () => {
render(component);

const platformButton = await screen.getByRole('button');
const popover = await screen.getByRole('navigation');
const popover = screen.getByTestId(popoverTestId);
const popoverFirstItem = popover.children[0].children[1];
userEvent.click(platformButton);
userEvent.tab();
Expand All @@ -84,19 +91,31 @@ describe('PlatformNavigator', () => {
});

it('should use current pathname when platform exists for that path', async () => {
render(<PlatformNavigator currentPlatform={'react'} isGen1={false} />);
render(
<PlatformNavigator
testId={testId}
currentPlatform={'react'}
isGen1={false}
/>
);

const popover = await screen.getByRole('navigation');
const popover = screen.getByTestId(popoverTestId);
const popoverFirstItem = popover.children[0].children[0];
expect(popoverFirstItem.children[0].getAttribute('href')).toBe(
'/react/build-ui/figma-to-code'
);
});

it('should use platform root url when platform does not exist for current pathname', async () => {
render(<PlatformNavigator currentPlatform={'react'} isGen1={false} />);
render(
<PlatformNavigator
testId={testId}
currentPlatform={'react'}
isGen1={false}
/>
);

const popover = await screen.getByRole('navigation');
const popover = screen.getByTestId(popoverTestId);

// Flutter
const popoverFirstItem = popover.children[0].children[6];
Expand Down
6 changes: 4 additions & 2 deletions src/components/PlatformNavigator/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,13 @@ import {
type PlatformNavigatorProps = {
currentPlatform: Platform;
isGen1: boolean;
testId?: string;
};

export function PlatformNavigator({
currentPlatform,
isGen1
isGen1,
testId
}: PlatformNavigatorProps) {
const { pathname } = useRouter();

Expand Down Expand Up @@ -57,7 +59,7 @@ export function PlatformNavigator({
{platformTitle}
</Popover.Trigger>
<Popover.List
ariaLabel="Supported frameworks and languages"
testId={testId ? `${testId}-popoverList` : ''}
anchor="left"
fullWidth={true}
>
Expand Down
9 changes: 1 addition & 8 deletions src/components/Popover/PopoverList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,9 @@ import { usePopover } from './usePopover';
interface PopoverListProps extends ViewProps {
anchor?: 'left' | 'right';
fullWidth?: boolean;
/**
* @description
* Accessible label for the nav item. Required since we are using a <nav> element.
*/
ariaLabel: string;
}

export const PopoverList = ({
ariaLabel,
children,
className,
anchor = 'right',
Expand All @@ -33,8 +27,7 @@ export const PopoverList = ({
},
className
)}
as="nav"
aria-label={ariaLabel}
as="div"
id={navId}
tabIndex={-1}
ref={contentRef}
Expand Down
22 changes: 7 additions & 15 deletions src/components/Popover/__tests__/Popover.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,12 @@ import userEvent from '@testing-library/user-event';

describe('Popover', () => {
const popoverTestId = 'popoverTestId';
const popoverListTestId = 'popoverListTestId';
const triggerLabel = 'Popover trigger';
const navLabel = 'Test list';
const popover = (
<Popover testId={popoverTestId}>
<Popover.Trigger>{triggerLabel}</Popover.Trigger>
<Popover.List ariaLabel={navLabel}>
<Popover.List testId={popoverListTestId}>
<Popover.ListItem href="">List item 1</Popover.ListItem>
<Popover.ListItem href="">List item 2</Popover.ListItem>
<Popover.ListItem href="">List item 3</Popover.ListItem>
Expand All @@ -25,7 +25,7 @@ describe('Popover', () => {
const popoverTrigger = screen.getByRole('button', {
name: triggerLabel
});
const popoverList = screen.getByRole('navigation', { name: navLabel });
const popoverList = screen.getByTestId(popoverListTestId);

expect(popoverWrapper).toBeInTheDocument();
expect(popoverTrigger).toBeInTheDocument();
Expand All @@ -37,9 +37,7 @@ describe('Popover', () => {
const button = await screen.findByRole('button', {
name: triggerLabel
});
const dropdown = await screen.findByRole('navigation', {
name: navLabel
});
const dropdown = screen.getByTestId(popoverListTestId);

userEvent.click(button);
expect(dropdown.classList).toContain('popover--expanded');
Expand All @@ -54,9 +52,7 @@ describe('Popover', () => {
const button = await screen.findByRole('button', {
name: triggerLabel
});
const dropdown = await screen.findByRole('navigation', {
name: navLabel
});
const dropdown = screen.getByTestId(popoverListTestId);

userEvent.click(document.body);
expect(dropdown.classList).not.toContain('popover--expanded');
Expand All @@ -73,9 +69,7 @@ describe('Popover', () => {
const button = await screen.findByRole('button', {
name: triggerLabel
});
const dropdown = await screen.findByRole('navigation', {
name: navLabel
});
const dropdown = screen.getByTestId(popoverListTestId);
const externalButton = await screen.findByRole('button', {
name: 'External button'
});
Expand Down Expand Up @@ -106,9 +100,7 @@ describe('Popover', () => {
const button = await screen.findByRole('button', {
name: triggerLabel
});
const dropdown = await screen.findByRole('navigation', {
name: navLabel
});
const dropdown = screen.getByTestId(popoverListTestId);
const externalButton = await screen.findByRole('button', {
name: 'External button'
});
Expand Down