WIP: React Tabs

[priley86]

Closes #52

Status:
Accessibility Review

What:

• Adds React Tabs components for basic tabs, justified tabs, patternfly tabs, secondary tabs
• Adds starter Nav components which are used internally by the React Tabs. These can be enhanced in subsequent PRs

React Storybook (see Tabs & Navbar stories):
https://rawgit.com/priley86/patternfly-react/tabs-storybook/index.html?knob-Label=Danger%20Will%20Robinson%21&knob-Tabs%20Active%20Tab=1&knob-Justified%20Active%20Tab=1&selectedKind=Tabs&selectedStory=Tabs&full=0&down=1&left=1&panelRight=0&downPanel=storybooks%2Fstorybook-addon-knobs

[jgiardino]

For the use case where <TabRowContents> would be included to display additional contents (e.g. buttons) on the right side of the tab row, is it possible to conditionally define the default classes to include pf-tabrow depending on whether or not <TabRowContents> is included? It would be nice to avoid requiring the dev to know that the class pf-tabrow needs to be defined if they decide to include.

[priley86]

@jgiardino i've updated the logic here in Tabs to check for a TabRowContents rendered child and apply the pf-tabrow class if found. Typically i've used state/props to pass down attributes, but since we are modularizing this for the consumer, I think it's OK (I think React Bootstrap does this sort of thing after reviewing it further).

I'm not sure if this is an acceptable React pattern though. @jtomasek any thoughts on this approach?

[jgiardino]

Please remove ul from all the selectors. Let's just use classes for selectors when possible.

[priley86]

done ;)

[jgiardino]

This is awesome, @priley86! I'm excited to test this out in my own project. I included a couple of quick comments, but I still need to review the role attributes as they related to accessibility. I also chatted with @maryclarke off-thread about the use case of including contents (e.g. buttons) on the right side of the tab row and if that should be added as a pattern to the pf library, so hopefully we'll have some feedback on that soon.

I also noticed that the css doesn't work as expected when including contents on the right and using the class nav-tabs-pf so I need to review that a bit more to figure out how to fix, assuming that's an option we want to enable.

[priley86]

@jgiardino added tests, a11y attributes to the stories, and i've enhanced the Tabs api to support the following props:

• className: overrides the ul class with whatever you want
• justified: adds the nav-justified class if attribute is present
• pfStyle: adds the nav-tabs-pf class if attribute is present

I think this will be pretty flexible...

Let me know if you have anything else on this!

[jtomasek]

What is the difference between this Dropdown and NavDropdown used below?

[priley86]

Unfortunately, there were style differences when the NavDropdown was used (React Bootstrap's component was not all that flexible). I'm leaning towards just using Dropdown only (it seems more versatile), or writing our own NavDropdown component. WDYT?

[priley86]

@jgiardino let me know any thoughts once you have design feedback 🎨 😄

[jgiardino]

After further discussion with Matt Carrano and @maryclarke, we are not planning to support actions inline in the tab row, due to issues with responsive design and given that there are other areas within the page where actions can be placed.

However, including elements besides text in the tab label (e.g. badge, icon) is still something we’d like to support.

Regarding accessibility, when I review the following design systems:

• https://polaris.shopify.com/components/navigation/tabs#navigation
• https://lightningdesignsystem.com/components/tabs/

they apply tab-index="-1" to the non-selected tabs, and utilize the arrow keys to change focus to a non-selected tab. I’m assuming the benefit of this implementation is to avoid forcing the user to tab through all the tabs before getting to the contents of the selected tab.

I see that you have tab-index="-1" but I’m not able to use the arrow keys to shift focus. Was there another method that you had implemented? If not, how simple would it be to enable the arrow keys to toggle the tab focus?

While I think the arrow key solution is nice, if this is more than you want to work on at the moment, then an alternate, simpler solution would be to just enable basic tab order to make non-selected tabs keyboard accessible by using the tab key. I think you would do this by just removing tab-index="-1"

Also, some may argue that the simpler tab key solution is what we should opt for anyway, since it doesn’t require additional documentation and uses standard browser-keyboard support. So I’m interested in what others think about this. @andresgalante @maryclarke any thoughts on this?

Finally, without being able to click on the tabs, I’m not able to really see what happens relative to scrolling. For example, would the page jump to the top, or scroll to the tab contents, or not change scroll position at all? This may just be something we watch after it has been merged and tested in an app, and revisit later if necessary.

[priley86]

thanks @jgiardino! I'm off for PTO next week so I'll have to get back on this one after I return.

I'm not positive how easy it will be to override the React Bootstrap components to change tab-index, but I will take a look when I return. We should be able to extend it if needed or worst case propose a change upstream.

[jgiardino]

@priley86,
Interesting that in the Nav component in React Bootstrap (i.e. <Nav bsStyle="tabs" ...>), they don't use tab-index="-1", whereas the Tabs component does (i.e. <Tabs ...>). I'm curious what their reason is for that.

Have you tested whether the ability to select a tab with an arrow key is already enabled? I'm wondering if that just isn't something that we can test in the context of storybook.

[dabeng]

Hi @priley86 , it will be better to add necessary props edit fields in Knobs tab.