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

feat: skip links #3317

Merged
merged 5 commits into from
Jul 12, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/breezy-wombats-hide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@twilio-paste/codemods': patch
'@twilio-paste/core': patch
---

[Codemods] include new skiplinks export from Sidebar
8 changes: 8 additions & 0 deletions .changeset/cyan-baboons-tie.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
---
'@twilio-paste/account-switcher': patch
'@twilio-paste/menu': patch
'@twilio-paste/product-switcher': patch
'@twilio-paste/core': patch
---

Fixed incorrect prop types for menu based components
6 changes: 6 additions & 0 deletions .changeset/lemon-birds-cover.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@twilio-paste/sidebar': minor
'@twilio-paste/core': minor
---

[Sidebar] Included skip link functionality, allowing consumers an easy way to create application navigation skip links. Added three new required props `mainContentSkipLinkID`, `sidebarNavigationSkipLinkID` and `topbarSkipLinkID`. Plus three optional internationalization props to translate the skip link text
6 changes: 6 additions & 0 deletions .changeset/tiny-news-hide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
'@twilio-paste/topbar': minor
'@twilio-paste/core': minor
---

[Topbar] Enable application navigation by providing an ID to the topbar for users to jump to via skiplink navigation.
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ const AccountSwitcherBadge = React.forwardRef<HTMLButtonElement, AccountSwitcher
}
);

// omit variant from MenuBadgePropTypes because it conflicts with AccountSwitcherBadge
const {variant, ...accountSwitcherPropTypes} = MenuBadgePropTypes;
Copy link
Contributor

Choose a reason for hiding this comment

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

question: No error for variant being unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We set it in the component above. AccountSwitcher is a subset of Menu, So I want to reuse the proptypes from Menu as much as I can, but because we set variant inside account switcher, the user can't so I don't want to error for them

AccountSwitcherBadge.displayName = 'AccountSwitcherBadge';
AccountSwitcherBadge.propTypes = MenuBadgePropTypes;
AccountSwitcherBadge.propTypes = accountSwitcherPropTypes;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Throwing errors in test and storybook

export {AccountSwitcherBadge};
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ const MenuMock: React.FC<React.PropsWithChildren<{groupRef?: React.Ref<HTMLDivEl
Search with Bing
</MenuItem>
</MenuGroup>
<MenuItemCheckbox {...menu} data-testid="checkboxmenuitem" name="formatting" value="wrap">
<MenuItemCheckbox {...menu} variant="destructive" data-testid="checkboxmenuitem" name="formatting" value="wrap">
Check Wrap
</MenuItemCheckbox>
<MenuItemCheckbox {...menu} name="formatting" value="no-wrap">
Expand Down
2 changes: 1 addition & 1 deletion packages/paste-core/components/menu/src/MenuItem.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const MenuItem = React.forwardRef<HTMLDivElement, MenuItemProps>(

export const MenuItemPropTypes = {
href: PropTypes.string,
variant: PropTypes.oneOf([Object.values(MenuItemVariants)]),
variant: PropTypes.oneOf(Object.values(MenuItemVariants)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Throwing errors in test and storybook

disabled: PropTypes.bool,
id: PropTypes.string,
onClick: PropTypes.func,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const MenuItemCheckbox = React.forwardRef<HTMLDivElement, MenuItemCheckboxProps>

export const MenuItemCheckboxPropTypes = {
href: PropTypes.string,
variant: PropTypes.oneOf([Object.values(MenuItemVariants)]),
variant: PropTypes.oneOf(Object.values(MenuItemVariants)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Throwing errors in test and storybook

disabled: PropTypes.bool,
id: PropTypes.string,
onClick: PropTypes.func,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ const ProductSwitcherButton = React.forwardRef<HTMLButtonElement, ProductSwitche
}
);

// omit variant and size from MenuButtonPropTypes as we set them intenrally
const {variant, size, ...productSwitcherButtonPropTypes} = MenuButtonPropTypes;
ProductSwitcherButton.displayName = 'ProductSwitcherButton';
ProductSwitcherButton.propTypes = MenuButtonPropTypes;
ProductSwitcherButton.propTypes = productSwitcherButtonPropTypes;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Throwing errors in test and storybook

export {ProductSwitcherButton};
56 changes: 40 additions & 16 deletions packages/paste-core/components/sidebar/__tests__/index.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,13 @@ const MockPushSidebar = ({
}): JSX.Element => {
return (
<Theme.Provider theme="twilio">
<Sidebar aria-label="main" collapsed={collapsed} variant={variant}>
<Sidebar
topbarSkipLinkID="topbar"
mainContentSkipLinkID="main"
sidebarNavigationSkipLinkID="nav"
collapsed={collapsed}
variant={variant}
>
<SidebarHeader>
<SidebarHeaderIconButton as="button">
<ProductFlexIcon size="sizeIcon20" decorative={false} title="Go to Flex product homepage" />
Expand All @@ -54,7 +60,13 @@ const MockOverlaySidebar = ({
}): JSX.Element => {
return (
<Theme.Provider theme="twilio">
<Sidebar aria-label="main" collapsed={collapsed} variant={variant}>
<Sidebar
topbarSkipLinkID="topbar"
mainContentSkipLinkID="main"
sidebarNavigationSkipLinkID="nav"
collapsed={collapsed}
variant={variant}
>
<SidebarHeader>
<SidebarHeaderIconButton as="button">
<ProductFlexIcon size="sizeIcon20" decorative={false} title="Go to Flex product homepage" />
Expand All @@ -79,25 +91,25 @@ describe('Sidebar', () => {
describe('Push Sidebar', () => {
it('should have an id', () => {
render(<MockPushSidebar collapsed />);
const nav = screen.getByRole('navigation');
const nav = screen.getByRole('complementary');
expect(nav).toHaveAttribute('id');
});

it('should render expanded', () => {
render(<MockPushSidebar collapsed={false} />);
const nav = screen.getByRole('navigation');
const nav = screen.getByRole('complementary');
expect(nav.style.width).toBe('15rem');
});

it('should render expanded by default', () => {
render(<MockPushSidebar />);
const nav = screen.getByRole('navigation');
const nav = screen.getByRole('complementary');
expect(nav.style.width).toBe('15rem');
});

it('should render compact width', () => {
render(<MockPushSidebar collapsed={true} variant="compact" />);
const nav = screen.getByRole('navigation');
const nav = screen.getByRole('complementary');
expect(nav.style.width).toBe('4.75rem');
});
});
Expand All @@ -108,25 +120,25 @@ describe('Sidebar', () => {
describe('Overlay Sidebar', () => {
it('should have an id', () => {
render(<MockOverlaySidebar collapsed />);
const nav = screen.getByRole('navigation');
const nav = screen.getByRole('complementary');
expect(nav).toHaveAttribute('id');
});

it('should render expanded', () => {
render(<MockOverlaySidebar collapsed={false} />);
const nav = screen.getByRole('navigation');
const nav = screen.getByRole('complementary');
expect(nav.style.width).toBe('15rem');
});

it('should render expanded by default', () => {
render(<MockOverlaySidebar />);
const nav = screen.getByRole('navigation');
const nav = screen.getByRole('complementary');
expect(nav.style.width).toBe('15rem');
});

it('should render compact width', () => {
render(<MockOverlaySidebar collapsed={true} variant="compact" />);
const nav = screen.getByRole('navigation');
const nav = screen.getByRole('complementary');
expect(nav.style.width).toBe('4.75rem');
});
});
Expand All @@ -138,7 +150,7 @@ describe('Sidebar', () => {
it('should have aria-expanded and aria-controls set correctly when collapsed', async () => {
render(<MockOverlaySidebar collapsed />);
const toggleButton = screen.getAllByRole('button')[1];
const nav = screen.getByRole('navigation');
const nav = screen.getByRole('complementary');
expect(toggleButton.getAttribute('aria-controls')).toEqual(nav.getAttribute('id'));
expect(toggleButton.getAttribute('aria-expanded')).toEqual('false');
expect(toggleButton.textContent).toBe('Open sidebar');
Expand All @@ -147,7 +159,7 @@ describe('Sidebar', () => {
it('should have aria-expanded and aria-controls set correctly when expanded', async () => {
render(<MockOverlaySidebar collapsed={false} />);
const toggleButton = screen.getAllByRole('button')[1];
const nav = screen.getByRole('navigation');
const nav = screen.getByRole('complementary');
expect(toggleButton.getAttribute('aria-controls')).toEqual(nav.getAttribute('id'));
expect(toggleButton.getAttribute('aria-expanded')).toEqual('true');
expect(toggleButton.textContent).toBe('Close sidebar');
Expand Down Expand Up @@ -233,7 +245,13 @@ describe('Sidebar', () => {
},
}}
>
<Sidebar aria-label="main" variant="compact" data-testid="aaa">
<Sidebar
topbarSkipLinkID="topbar"
mainContentSkipLinkID="main"
sidebarNavigationSkipLinkID="nav"
variant="compact"
data-testid="aaa"
>
<Box color="colorTextInverse">Sidebar header</Box>
<SidebarBetaBadge as="span">Beta</SidebarBetaBadge>
<SidebarFooter data-testid="collapseButtonWrapper">
Expand All @@ -247,7 +265,7 @@ describe('Sidebar', () => {
</SidebarPushContentWrapper>
</CustomizationProvider>
);
const nav = screen.getByRole('navigation');
const nav = screen.getByRole('complementary');
expect(nav).toHaveStyleRule('margin', '1rem');
expect(nav).toHaveStyleRule('background-color', 'rgb(2, 99, 224)');

Expand Down Expand Up @@ -284,7 +302,13 @@ describe('Sidebar', () => {
},
}}
>
<Sidebar aria-label="main" variant="compact" element="XSIDE">
<Sidebar
topbarSkipLinkID="topbar"
mainContentSkipLinkID="main"
sidebarNavigationSkipLinkID="nav"
variant="compact"
element="XSIDE"
>
<Box color="colorTextInverse">Sidebar header</Box>
<SidebarBetaBadge as="span" element="XSIDE_BETA_BADGE">
Beta
Expand All @@ -304,7 +328,7 @@ describe('Sidebar', () => {
</SidebarPushContentWrapper>
</CustomizationProvider>
);
const nav = screen.getByRole('navigation');
const nav = screen.getByRole('complementary');
expect(nav).toHaveStyleRule('margin', '1rem');
expect(nav).toHaveStyleRule('background-color', 'rgb(2, 99, 224)');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,14 +38,20 @@ const MockPushSidebarWithNavigation = ({
/* eslint-disable react/jsx-max-depth */
return (
<Theme.Provider theme="twilio">
<Sidebar aria-label="Main" collapsed={collapsed} variant="compact">
<Sidebar
topbarSkipLinkID="topbar"
mainContentSkipLinkID="main"
sidebarNavigationSkipLinkID="nav"
collapsed={collapsed}
variant="compact"
>
<SidebarHeader>
<SidebarHeaderIconButton as="button">
<ProductFlexIcon size="sizeIcon20" decorative={false} title="Go to Flex product homepage" />
</SidebarHeaderIconButton>
<SidebarHeaderLabel>Twilio Flex</SidebarHeaderLabel>
</SidebarHeader>
<SidebarNavigation data-testid="nav-wrapper">
<SidebarNavigation aria-label="main">
<SidebarNavigationItem
href="https://google.com"
data-testid="nav-item-button"
Expand Down Expand Up @@ -141,7 +147,7 @@ describe('SidebarNavigation', () => {
const onClick: jest.Mock = jest.fn(() => {});

render(<MockPushSidebarWithNavigation collapsed onClick={onClick} />);
const wrapper = screen.getByTestId('nav-wrapper');
const wrapper = screen.getByRole('navigation');
const buttonIcon = screen.getByTestId('nav-item-button');
const anchorIcon = screen.getByTestId('nav-item-anchor');
const disclosure = screen.getByTestId('nav-item-disclosure');
Expand Down Expand Up @@ -174,7 +180,7 @@ describe('SidebarNavigation', () => {
const onClick: jest.Mock = jest.fn(() => {});

render(<MockPushSidebarWithNavigation collapsed={false} onClick={onClick} />);
const wrapper = screen.getByTestId('nav-wrapper');
const wrapper = screen.getByRole('navigation');
const disclosure = screen.getByTestId('nav-item-disclosure');
const disclosureHeading = screen.getByTestId('nav-item-disclosure-heading');
const disclosureContent = screen.getByTestId('nav-item-disclosure-content');
Expand Down Expand Up @@ -224,8 +230,14 @@ describe('SidebarNavigation', () => {
SIDEBAR_NAVIGATION_DISCLOSURE_CONTENT: {margin: 'space100'},
}}
>
<Sidebar aria-label="main" variant="compact" data-testid="aaa">
<SidebarNavigation data-testid="nav-wrapper">
<Sidebar
topbarSkipLinkID="topbar"
mainContentSkipLinkID="main"
sidebarNavigationSkipLinkID="nav"
variant="compact"
data-testid="aaa"
>
<SidebarNavigation aria-label="main">
<SidebarNavigationItem href="http://www.google.com" selected>
AnchorItem Selected
</SidebarNavigationItem>
Expand All @@ -250,7 +262,7 @@ describe('SidebarNavigation', () => {
</Sidebar>
</CustomizationProvider>
);
const nav = screen.getByTestId('nav-wrapper');
const nav = screen.getByRole('navigation');
const disclosure = screen.getByTestId('disclosure');
const disclosureHeading = screen.getByRole('button', {name: 'Heading'});
const disclosureHeadingWrapper = disclosureHeading.parentElement;
Expand Down Expand Up @@ -283,8 +295,14 @@ describe('SidebarNavigation', () => {
NAVIGATION_DISCLOSURE_CONTENT: {margin: 'space100'},
}}
>
<Sidebar aria-label="main" variant="compact" data-testid="aaa">
<SidebarNavigation element="NAVIGATION" data-testid="nav-wrapper">
<Sidebar
topbarSkipLinkID="topbar"
mainContentSkipLinkID="main"
sidebarNavigationSkipLinkID="nav"
variant="compact"
data-testid="aaa"
>
<SidebarNavigation aria-label="main" element="NAVIGATION">
<SidebarNavigationItem element="NAVIGATION_ITEM_ANCHOR" href="http://www.google.com" selected>
AnchorItem Selected
</SidebarNavigationItem>
Expand Down Expand Up @@ -321,7 +339,7 @@ describe('SidebarNavigation', () => {
</Sidebar>
</CustomizationProvider>
);
const nav = screen.getByTestId('nav-wrapper');
const nav = screen.getByRole('navigation');
const disclosure = screen.getByTestId('disclosure');
const disclosureHeading = screen.getByRole('button', {name: 'Heading'});
const disclosureHeadingWrapper = disclosureHeading.parentElement;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import * as React from 'react';
import {render, screen} from '@testing-library/react';

import {SidebarSkipLinks} from '../src/SidebarSkipLinks';

describe('SkipLinks', () => {
it('should render three links', () => {
render(<SidebarSkipLinks mainContentSkipLinkID="ID1" sidebarNavigationSkipLinkID="ID2" topbarSkipLinkID="ID3" />);

const renderedLinks = screen.getAllByRole('link');

expect(renderedLinks.length).toBe(3);
expect(renderedLinks[0]).toHaveAttribute('href', '#ID1');
expect(renderedLinks[1]).toHaveAttribute('href', '#ID2');
expect(renderedLinks[2]).toHaveAttribute('href', '#ID3');
expect(screen.getByRole('link', {name: /skip to content/i})).toBeInTheDocument();
expect(screen.getByRole('link', {name: /skip to navigation/i})).toBeInTheDocument();
expect(screen.getByRole('link', {name: /skip to topbar/i})).toBeInTheDocument();
});

it('should render translated links', () => {
render(
<SidebarSkipLinks
mainContentSkipLinkID="ID1"
sidebarNavigationSkipLinkID="ID2"
topbarSkipLinkID="ID3"
i18nMainContentSkipLinkText="translated main"
i18nTopbarSkipLinkText="translated topbar"
i18nNavigationSkipLinkText="translated nav"
/>
);

expect(screen.getByRole('link', {name: /translated main/i})).toBeInTheDocument();
expect(screen.getByRole('link', {name: /translated nav/i})).toBeInTheDocument();
expect(screen.getByRole('link', {name: /translated topbar/i})).toBeInTheDocument();
});

it('should render default element names', () => {
render(<SidebarSkipLinks mainContentSkipLinkID="ID1" sidebarNavigationSkipLinkID="ID2" topbarSkipLinkID="ID3" />);
expect(screen.getByRole('link', {name: /skip to content/i}).dataset.pasteElement).toBe('SIDEBAR_SKIPLINKS_LINK');
expect(screen.getByRole('link', {name: /skip to navigation/i}).dataset.pasteElement).toBe('SIDEBAR_SKIPLINKS_LINK');
expect(screen.getByRole('link', {name: /skip to topbar/i}).dataset.pasteElement).toBe('SIDEBAR_SKIPLINKS_LINK');
});

it('should render custom element names', () => {
render(
<SidebarSkipLinks
mainContentSkipLinkID="ID1"
sidebarNavigationSkipLinkID="ID2"
topbarSkipLinkID="ID3"
element="CUSTOMLINKS"
/>
);
expect(screen.getByRole('link', {name: /skip to content/i}).dataset.pasteElement).toBe('CUSTOMLINKS_LINK');
expect(screen.getByRole('link', {name: /skip to navigation/i}).dataset.pasteElement).toBe('CUSTOMLINKS_LINK');
expect(screen.getByRole('link', {name: /skip to topbar/i}).dataset.pasteElement).toBe('CUSTOMLINKS_LINK');
});
});