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

Tabs: add "with links" Storybook example #67699

Closed
wants to merge 2 commits into from
Closed

Tabs: add "with links" Storybook example #67699

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f26d8a5
Tabs: add with links Storybook example
ciampo Dec 6, 2024
7b6d3e9
Improve Tabs.Tab CSS reset styles
ciampo Dec 6, 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
129 changes: 128 additions & 1 deletion packages/components/src/tabs/stories/index.story.tsx
View file
Open in desktop
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hope the example added here can represent how Tabs could be integrated with links and a router. There are of course many other ways, and it would largely depend on the routing library (for example, look at ariakit's examples with react router and next.js).

What do folks think?

Copy link
Member

@mirka mirka Dec 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not worried about people being able to integrate specific routing libraries, as much as I'm worried about people being able to get the accessibility details right for their use case. I think we should at least have a description outlining the accessibility behaviors you need to look for when implementing a given use case:

  • When the link doesn't push an item to browser history, but the state can be navigated to using an href (the current example output <a href="foo" role="tab"> is fine)
  • When the link does push an item to browser history (it should be <a href="foo"> and no tab semantics — do we even support this yet?)

I think these are the two main cases but I'm not 100% sure on the accessibility requirements. For example the Next.js example on Ariakit seems lacking to me, as a screen reader user would not be able to perceive that the browser window navigated to a new page (@diegohaz Thoughts?).

We can see examples of the latter case (tab-like styles on what are just semantically nav links):

Main navigation tabs on GitHub PR navigation tabs on GitHub

Copy link
Member

Choose a reason for hiding this comment

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

I agree that the Ariakit examples on Tabs and routing need clearer instructions on how to use them.

In the end, this is about design.

ARIA roles guide users on interacting with elements. Rather than focusing on these roles, designers should first consider how users will engage with a widget and the rest of the page, always aiming for the best user experience. If it's determined that having navigation as a single tab stop navigable with arrow keys improves user experience, with the URL state change being secondary, then you should find the role that best represents this interaction model (which could be tab), just as you would choose visuals that help sighted users intuitively understand how to interact with the elements the way it's designed.

Design comes first. Then, semantic HTML. If needed, ARIA follows. Everyone involved in design should prioritize accessibility, not because of ARIA, but because it's fundamental to UX. If design fails, everything else is compromised.

That being said, you can't cover all of this in the docs, so it might be better to describe various scenarios that people can refer to for their specific use cases.

When designing websites, you generally want your navigation areas to be lists of semantic, tabbable links, as this is what most users are familiar with. However, if it's a secondary widget, like a sidebar section where changing tabs only affects the URL search params, this approach might differ. In a desktop-like app, such as an online code editor, users generally expect to navigate past the open file list with a single tab and move through them using the arrow keys, as that's how most code editors work. You can choose to update the URL state to represent the selected tab, which could facilitate sharing, but this is a secondary effect. If your target audience consists of VIM users, you'll probably need to design it in a way that's more familiar to them.

Again, design comes first.

Copy link
Contributor Author

@ciampo ciampo Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When the link doesn't push an item to browser history, but the state can be navigated to using an href (the current example output is fine)

Is an <a /> that doesn't push an item to browser history a good pattern to recommend in general? Why not use a button in that case?

I'm struggling to understand what we think a good "Tabs with links" example is. Tabs offers, at a high level, the semantics of tablist/tab/tabpanel, composite-like keyboard navigation for the tablist, and a certain look/feel for its tabs (including the indicator).

For traditional "lists of links", the semantics are list > listitem > link — does it make any sense to override tabs semantics? What about other attributes like aria-controls, aria-selected, etc? It just feels like, semantically, anything that can't work with tabs semantics would not be well-aligned with the component.

In terms of keyboard navigation, links are usually expected to be navigated with the tab key.

The look&feel really feels like the "added value" that the component would offer to a consumer.

In short, I'm struggling to identify what is a good canonical example that we can use for "tabs with links".

@mirka @diegohaz @tyxla what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

In short, I'm struggling to identify what is a good canonical example that we can use for "tabs with links".

Same. That's exactly why I left this comment last week.

Copy link
Member

Choose a reason for hiding this comment

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

I agree that we probably shouldn't support the "links that look like Tabs" pattern. Even though it has been requested before, it's usually bad for user expectations when an identical-looking piece of UI can behave very differently.

The only use case I can see as potentially useful, is this one:

• When the link doesn't push an item to browser history, but the state can be navigated to using an href

In other words, cases when each tabpanel can be accessed with a unique URL, but selecting a tab from the tablist doesn't alter the browser history stack. The benefit of using <a role="tab"> here is that mouse users can command-click to open in a new window, or right-click to copy the URL. And I don't think there are any downsides, since screen reader users won't be confused by the history stack changing unexpectedly when clicking on what is announced as a tab.

Copy link
Contributor Author

@ciampo ciampo Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure if the suggested use-case is "good practice" and frequent enough to warrant a dedicated Storybook example, at least for now. If no one is strongly opposed, I suggest dropping this effort for the time being, and revisiting if needed in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the use case is a bit contrived 😅 Totally fine with dropping.

Or, since we do get these requests from time to time, it may be worth documenting somewhere that we don't recommend people try using Tabs with links, because of all these reasons.

Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@ import type { Meta, StoryFn } from '@storybook/react';
* WordPress dependencies
*/
import { wordpress, more, link } from '@wordpress/icons';
import { useState } from '@wordpress/element';
import {
useState,
forwardRef,
createContext,
useContext,
} from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -492,3 +497,125 @@ const TabGetsRemovedTemplate: StoryFn< typeof Tabs > = ( props ) => {
);
};
export const TabGetsRemoved = TabGetsRemovedTemplate.bind( {} );

const WITH_LINKS_PAGES = [
{
title: 'Page 1',
url: 'page1',
},
{
title: 'Page 2',
url: 'page2',
},
{
title: 'Page 3',
url: 'page3',
},
];

const SimpleRouterContext = createContext< {
currentPath: string | undefined;
goTo: ( path: string | undefined ) => void;
} >( {
currentPath: undefined,
goTo: () => {},
} );

const SimpleRouter = ( {
children,
initialRoute = '/',
}: {
children: React.ReactNode;
initialRoute?: string;
} ) => {
const [ currentPath, setCurrentPath ] = useState< string | undefined >(
initialRoute
);

const goTo = ( path: string | undefined ) => {
setCurrentPath( path );
};

return (
<SimpleRouterContext.Provider value={ { currentPath, goTo } }>
{ children }
</SimpleRouterContext.Provider>
);
};

const SimpleRouterLink = forwardRef(
(
{
to,
children,
onClick,
style,
...restProps
}: {
to: string;
children?: React.ReactNode;
} & React.HTMLProps< HTMLAnchorElement >,
ref: React.ForwardedRef< HTMLAnchorElement >
) => {
const { goTo } = useContext( SimpleRouterContext );

return (
<a
Copy link
Member

Choose a reason for hiding this comment

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

If we're encouraging folks to use links, doesn't that stray away from Tabs in general? And if we don't want to actively encourage use of links, isn't this example close enough to the one for controlled mode?

href={ to }
ref={ ref }
onClick={ ( event ) => {
event.preventDefault();
goTo( to );
onClick?.( event );
} }
style={ {
...style,
textDecoration: 'none',
} }
{ ...restProps }
>
{ children }
</a>
);
}
);

const WithLinksTabs = ( props: React.ComponentProps< typeof Tabs > ) => {
const { currentPath, goTo } = useContext( SimpleRouterContext );

return (
<Tabs
selectedTabId={ currentPath }
onSelect={ ( newTabId ) => {
goTo( newTabId ?? undefined );
} }
Comment on lines +589 to +591
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 feels correct, even if it could be redundant since the SimpleRouterLink component also calls goTo internally. What do folks think:?

{ ...props }
>
<Tabs.TabList>
{ WITH_LINKS_PAGES.map( ( page ) => (
<Tabs.Tab
tabId={ page.url }
key={ page.url }
render={ <SimpleRouterLink to={ page.url } /> }
>
{ page.title }
</Tabs.Tab>
) ) }
</Tabs.TabList>
{ WITH_LINKS_PAGES.map( ( page ) => (
<Tabs.TabPanel tabId={ page.url } key={ page.url }>
<p>Selected tab: { page.title }</p>
</Tabs.TabPanel>
) ) }
</Tabs>
);
};

const WithLinksTemplate: StoryFn< typeof Tabs > = ( props ) => {
return (
<SimpleRouter initialRoute={ WITH_LINKS_PAGES[ 0 ].url }>
<WithLinksTabs { ...props } />
</SimpleRouter>
);
};
export const WithLinks = WithLinksTemplate.bind( {} );
2 changes: 2 additions & 0 deletions packages/components/src/tabs/styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,9 +151,11 @@ export const StyledTabList = styled( Ariakit.TabList )`
export const Tab = styled( Ariakit.Tab )`
& {
/* Resets */
appearance: auto;
border-radius: 0;
background: transparent;
border: none;
box-sizing: border-box;
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 effectively needed to make sure that the tab indicator measures the tab size correctly

box-shadow: none;

flex: 1 0 auto;
Expand Down
Loading