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] Initial migration of component #5604

Merged
merged 1 commit into from
Nov 22, 2016
Merged

[Tabs] Initial migration of component #5604

merged 1 commit into from
Nov 22, 2016

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Nov 20, 2016

  • Add some unit tests
  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

@newoga That was more challenging than I though. Your initial work was quite useful!

chrome-53 0 2785 143-linux

Close #5097.
Close #5534.
Close #5391.
Close #5370.
Close #5094.
Close #4528.
Close #4420.

@oliviertassinari oliviertassinari added component: tabs This is the name of the generic UI component, not the React module! next labels Nov 20, 2016
@muibot muibot added PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Nov 20, 2016
@oliviertassinari oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Nov 20, 2016
@nathanmarks
Copy link
Member

@oliviertassinari @newoga thanks for taking this massive component on!

This is looking really awesome. I have some initial feedback though regarding accessibility.

1. There needs to be properties for the relevant roles: tablist and tab.

<div role="tablist">
  <button role="tab">tab 1</button>
  <button role="tab">tab 2</button>
</div>

2. The tabs should have aria-selected and tabindex properties

<div role="tablist">
  <button role="tab" tabindex="0" aria-selected="true">tab 1</button>
  <button role="tab" tabindex="-1" aria-selected="false">tab 2</button>
</div>

3. A tablist widget needs to implement specific keyboard navigation techniques:

  • Tab - only the active tab is in the tab order. The user reaches the tabbed panel component by pressing the tab key until the active tab title receives focus.
  • Left Arrow - with focus on a tab, pressing the left arrow will move focus to the previous tab in the tab list and activate that tab. Pressing the left arrow when the focus is on the first tab in the tab list will move focus and activate the last tab in the list.
  • Right Arrow - with focus on a tab, pressing the right arrow will move focus to the next tab in the tab list and activate that tab. Pressing the right arrow when the focus is on the last tab in the tab list will move focus to and activate the first tab in the list.
  • Up arrow - behaves the same as left arrow in order to support vertical tabs
  • Down arrow - behaves the same as right arrow in order to support vertical tabs

You can see an example of similar functionality in these components (including resetting the tabindex when a widget is blurred):
https://github.com/callemall/material-ui/blob/next/src/Radio/RadioGroup.js
https://github.com/callemall/material-ui/blob/next/src/Menu/MenuList.js

(not 💯 perfect yet. specifically, have a couple of SR issues to address on the demos but the keyboard accessibility + focus management is there).

BTW I'm more than happy to help implement this. Also, I'm wondering if now is time to abstract some of this functionality away from these components? There are some small differences in how they need to behave so the abstraction would definitely take a bit of thinking.

Let me know if you want to discuss this abstraction or if you think we should just add the functionality to the component.

You can see a real world example of all of these accessibility features here:
https://plus.google.com/collections/featured

@oliviertassinari
Copy link
Member Author

@nathanmarks Thanks for the feedback regarding accessibility.
I have addressed the simplest points. I still have to solve the tabindex and the keyboard navigation.

Also, I'm wondering if now is time to abstract some of this functionality away from these components?

We would also need to address this issue at the BottomNavigation level. We have 4 components with a similar needs. I'm gonna look at how we could share some logic. I believe react-bootstrap is handling correctly this aspect.

@nathanmarks nathanmarks mentioned this pull request Nov 22, 2016
10 tasks
@oliviertassinari oliviertassinari merged commit 331ecbc into mui:next Nov 22, 2016
@oliviertassinari oliviertassinari deleted the next-tabs branch November 22, 2016 21:23
@oliviertassinari
Copy link
Member Author

I have updated #4795. I'm gonna merge this PR, the migration isn't finished yet.

@nathanmarks
Copy link
Member

@oliviertassinari Before we jump head first into the logic sharing, I want to make sure that we're handling the slightly different use cases properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tabs This is the name of the generic UI component, not the React module!
Projects
None yet
4 participants