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

Tabs: add "with links" Storybook example #67699

Closed
wants to merge 2 commits into from
Closed

Conversation

ciampo
Copy link
Contributor

@ciampo ciampo commented Dec 6, 2024

What?

Add an example to Tabs to show how to use the component with links and a sample router

Why?

This is a valid usecase and consumers could benefit from finding an example

How?

  • Added a new Storybook example, with a custom, stripped-down "memory" router
  • Added a couple of extra styles to make sure the component keeps working as expected

Testing Instructions

Review and test the new Storybook example, make sure it works as expected

@ciampo ciampo added [Type] Developer Documentation Documentation for developers [Package] Components /packages/components labels Dec 6, 2024
@ciampo ciampo requested a review from a team December 6, 2024 16:57
@ciampo ciampo self-assigned this Dec 6, 2024
@ciampo ciampo requested a review from ajitbohra as a code owner December 6, 2024 16:57
Copy link

github-actions bot commented Dec 6, 2024

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: ciampo <[email protected]>
Co-authored-by: mirka <[email protected]>
Co-authored-by: diegohaz <[email protected]>
Co-authored-by: tyxla <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

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

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

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

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.

Comment on lines +589 to +591
onSelect={ ( newTabId ) => {
goTo( newTabId ?? undefined );
} }
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:?

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

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?

@ciampo
Copy link
Contributor Author

ciampo commented Dec 19, 2024

As per this conversation, I'm closing this PR. I will open a new PR that focus on adding documentation to the component about why we don't recommend using it with links.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants