feat(tabs): add the pf tabs component

[dabeng]

What:
This PR adds the PatternFly Tabs pattern. To be used to close #52 (Tabs) once done.

Why: Because #49 involves many dated code snippets and conflicts, I create new PR for component tabs.

How:
Providing a tabs story of patternfly-react storybook based on react-bootstrap tabs component.

@priley86 , @jgiardino please review Storybook.

[priley86]

Yep.. we can close #49 now. Just left it up for future reference...

Thanks for following up so fast on this component @dabeng. I will try to review it some today.

[jgiardino]

Thanks for implementing this, @dabeng!

I did a quick pass at reviewing for keyboard support. The storybook example 'Tabs with dropdown' has some issues, where the dropdown menu is not keyboard accessible. It looks like this example is based on the react-bootstrap navbar component, but this component behaves differently.

For example, the react-bootstrap navbar component:

1. uses the Tab key to navigate between the tabs
2. uses the Enter key to expand/collapse the Dropdown
3. uses the Up and Down arrow keys to navigate between menu items

However, in the storybook example 'Tabs with dropdown', this component:

1. uses the arrow keys to navigate between tabs (Right or Down goes to the next tab, Left or Up goes to the previous tab)
2. the Down arrow key will expand the dropdown menu for Tab 3, and at the same time shift focus to Tab 1
3. there are no keys that can access the dropdown menu items
   • my guess here is that the Up and Down arrow keys are defined for keyboard navigation, but this is resulting in two event handlers for the Down arrow key when Tab 3 has focus: one to shift focus to the first menu item in the dropdown and another one to shift focus to Tab 1.

Can you confirm if the storybook example 'Tabs with dropdown' is in fact based on the react-bootstrap navbar component?

And if that is the case, do you know why the keyboard interaction is different between the two?

[priley86]

@dabeng can we contribute any of these a11y enhancements easily upstream to React Bootstrap? Maybe that would be worth investigating for future contributions...

[jgiardino]

Oooh, I just realized/remembered that there are two very similar react-bootstrap components, but they have different keyboard interactions:

• react-bootstrap navbar
  • uses Tab key to navigate between tabs
  • the dropdown component is keyboard accessible
• react-bootstrap tabs with dropdown
  • uses Arrow keys to navigate between tabs
  • the dropdown component is NOT keyboard accessible

And I'm guessing the storybook example for 'Tabs with dropdown' is based on react-bootstrap tabs with dropdown

So these seem like our options for this:

1. Use the react-bootstrap navbar component instead so that we have keyboard interaction that works.
   • If we use this component, is it possible to add classes so that the Tabs styles are used (specifically, nav-tabs for the <ul> element and active for the selected <li> element)?
2. Contribute a fix upsteam to react-bootstrap to fix react-bootstrap tabs with dropdown :-)
3. Defer implementing the pattern of 'tabs with dropdown' for a later PR.
   • Does anyone have any data on whether this pattern is used or needed.

Does anyone have any thoughts on how we should proceed with this one?

[dabeng]

Hi @jgiardino , tabs of react-bootstrap just followed WAI-ARIA Authoring Practices 1.1.

Tab key is enabled in react-bootstrap navs because there are not tabpanels or tabpanes in this case.

Tab key is disabled in react-bootstrap tabs with dropdown because there are usually focusable elements (typically links and form controls) within a tabpane or tabpanel like this demo shows. Instead, we turn to direction keys to toggle different tabs, and then arrow keys are disabled in component NavDropdown nested in TabContainer.

I reviewed the following related materials. Hope they could be helpful for you too 😊

• Missing tabIndex attribute using Tabs.js  react-bootstrap/react-bootstrap#2783
• https://www.w3.org/TR/wai-aria-practices/examples/tabs/tabs-1/tabs.html
• https://stackoverflow.com/questions/32911355/whats-the-tabindex-1-in-bootstrap-for/32912224

[jgiardino]

Thanks for adding the clarification and the references, @dabeng!

Regarding your comment above:

Instead, we turn to direction keys to toggle different tabs, and then arrow keys are disabled in component NavDropdown nested in TabContainer.

Using arrow direction keys works as expected for the tabs. But the fact that they are "disabled in component NavDropdown nested in TabContainer" is a bug, because this behavior prevents keyboard access to the dropdown menu.

For now, the recommendation on how to move forward with this PR is to:

• keep the implementation as you have it now, and
• include the following text in the storybook example Tabs with Dropdown:
  • "Note: This variation is not keyboard accessible and is not recommended for use where accessibility is a priority."

We can later remove this note when the react-bootstrap component is fixed upstream.

This recommendation is based on a discussion I had with some of the UXD members who contribute to other patternfly repos.

[dabeng]

Hi @jgiardino , thanks a lot for your great recommendation. I have added the following notes for the story of "Tabs with Dropdown".

[jgiardino]

Thanks for adding the note, @dabeng!
I don't see it in the published storybook example, though.

After chatting with some of the other patternfly contributors, we'd like to have the patternfly-react storybook examples include as many of the patternfly examples as possible, but not include anything that is not documented as part of patternfly. I created this list of specific updates needed based on that guideline.

1. For the storybook example Basic Tabs:
   1. the KNOBS should include the option to show the tabs with "Patternfly Style" where the class .nav-tabs-pf is applied.
   2. the KNOBS should not include the option to show the tabs as "pills"
      • Since "pills" is not a style that is used in the patternfly examples, then we should avoid showing them in the pf-react storybook examples
2. For the storybook example Secondary Tabs:
   1. the secondary navigation should always display using the Patternfly style.
      • The only variations that should display in the pf-react storybook example are the ones shown in the patternfly Tabs test page, under the section "PatternFly Examples" > ".nav-tabs + .nav-tabs-pf"
   2. the KNOBS should not include the option to show the tabs as "pills"

[priley86]

Can we also export/extend these React Bootstrap components in src/index.js to be consistent with other React Bootstrap components used?

[priley86]

I know we were just targeting Tabs w/ this PR, but I think it's good to go ahead and export/extend Nav components as the others if using them for these examples...

[dabeng]

Hi @jgiardino , you need to click "show info" button to check notes content. Beside, I have updated the PR to add "patternfly style" and remove "pills style". @priley86 , I abandoned import ... form 'react-bootstrap'.

[jgiardino]

I think it's fine to include this note under Show Info, but my concern is that it's a little buried and can be easily missed.

Can we also add this note to the description for this storybook variation? So that it displays like this:

And can we add this description to just the Tabs with Dropdown example and not the other Tabs examples?

[jgiardino]

Thanks for pointing out where the note was. I included an inline comment above about this.
And thank you for updating the KNOBS!

I reviewed the secondary navigation again, and there are a couple of issues I noticed:

1. Use of grid
   • The use of rows and columns is different than the patternfly test page layout showing tabs. Can the pf-react example be updated to more closely match the patternfly example?
   • The <Grid> component is missing from all of the storybook examples, however...
   • I'm not sure that the <Grid> components are needed at all for these examples
2. The html is different than how patternfly css expects it to be.
   • The patternfly css expects the primary and secondary tabs to be sibling elements, as shown below:

     <ul class="nav nav-tabs">...</ul>
     <ul class="nav nav-tabs nav-tabs-pf">...</ul>
     

     Without this html structure, the css will not get applied as it should.
   • But after removing the unnecessary grid components (as noted under issue 1 above), the example in the storybook has this html structure instead, which still breaks the css selectors for secondary nav due to the extra <div>s:

     <ul role="tablist" class="nav nav-tabs">...</ul>
     <div class="tab-content">
       <div id="secondary-tabs-1">
         <ul role="tablist" class="nav nav-tabs nav-tabs-pf">...</ul>
         <div class="tab-content">...</div>
       </div>
     </div>
     

     If the <Grid> components are included as in your storybook example, there are several more <div>s that separate the two <ul> elements.
   • As an alternate example, @priley86's PR for tab component does not have this issue. However he is also not including a <TabContent> component in his implementation, so i'm not sure if the html in his implementation would hold up or not after adding the interaction and tab contents.
   • Is it possible to update the components to address this html issue?

[dabeng]

Hi @jgiardino , thanks for deep review.

1. The following structure is simple and clean , but will got error message "React.Children.only expected to receive a single React element child". I think "TabContainer" component should need at least (and most) one child element.

    <TabContainer>
      <Nav>
        <NavItem>Tab 1</NavItem>
        <NavItem>Tab 2</NavItem>
        <NavItem>Tab 3</NavItem>
      </Nav>
      <TabContent animation>
        <TabPane>Tab 1 content</TabPane>
        <TabPane>Tab 2 content</TabPane>
        <TabPane>Tab 3 content</TabPane>
      </TabContent>
    </TabContainer>

2. In fact, I'm skeptical about pattternfly secondary tabs, secondary tabs are not nested in primary tabs, why call them secondary tabs. If the relation of primary tabs and secondary tabs is equal , what's the usage scenario of such parallel primary&secondary tabs?

3. If primary tabs and secondary tabs are parallel, their active tabs are irrelevant, but the UI layout will puzzle end users.

[jgiardino]

@dabeng You're right in all of your points.

For point # 1, this issue goes away with react 16, which may not be too far off for this repo. For now, my preference would be using a <div> to wrap the sibling components instead of adding the <Grid> components.

For points # 2 and 3, I agree with your points about the structure of the html not being semantic. The way your component implements the user interaction is correct. The true issue here is the way patternfly css selectors are written and the html structure that is expected—the css is too tightly coupled with the html structure instead of being applied purely by css classes. And your point about the html

In my previous comment, I was wondering if there is a way to structure the components so that we can match the html structure that is expected by the patternfly css. In other words, can the tabs be implemented in a way that still provides the same hierarchical/nested behavior and interaction that you have now, even if their underlying structure isn't?

I just wanted to double-check if we could resolve this issue in patternfly-react first. But if this isn't possible, please let me know and we can create an issue in patternfly core to update the css. (Although after you're previous comment, I'm definitely leaning more in favor of updating patternfly css instead)

[priley86]

please rebase as #137 today changes comma settings.

[dabeng]

Hi @priley86 , I have rebased the PR to following the new comma settings.

[dabeng]

Sorry @jgiardino, I have no idea where to place the secondary tabs markup of Tab 2.

On the other hand, if we just keep one secondary tabs, disable Tab 2 and Tab3, and remove all tab-panes, we can finally achieve the goal you described above. Do this solution make sense?

[dabeng]

@priley86 @jgiardino , any new concerns or comments about this PR?

[jgiardino]

On the other hand, if we just keep one secondary tabs, disable Tab 2 and Tab3, and remove all tab-panes, we can finally achieve the goal you described above. Do this solution make sense?

I'm sorry, I'm having trouble understanding this idea. Do you have an example (even if just a code snippet pasted in a comment)?

Although, it seems like the html that is implemented in patternfly core cannot be reproduced exactly in patternfly-react. If that really is the case, we can merge this as it is and open an issue in patternfly core to update the css so that it works with the solution we're able to implement.

What do you think?

[dabeng]

Hi @jgiardino , this is the demo which just includes nav section without tabcontent. And the following is source code:

    <div>
      <Nav bsStyle="tabs" activeKey="1">
        <NavItem eventKey="1" title="Tab 1">
          Tab 1
        </NavItem>
        <NavItem eventKey="2" title="Tab 2">
          Tab 2
        </NavItem>
        <NavItem eventKey="3" title="Tab 3">
          Tab 3
        </NavItem>
      </Nav>
      <Nav bsClass="nav nav-tabs nav-tabs-pf" activeKey="1.1">
        <NavItem eventKey="1.1">Tab A</NavItem>
        <NavItem eventKey="1.2">Tab B</NavItem>
        <NavItem eventKey="1.3">Tab C</NavItem>
      </Nav>
    </div>

Does this secondary tabs make sense?

[jgiardino]

Thank you for the demo example.

In the original storybook, the example included the ability to toggle the tab selection and show different contents. That example was great because it is pretty obvious for a product developer on how to consume the tabs and define the tab contents.

My concern with the new example of secondary tabs is that it's no longer obvious how to define the tab contents. I also question if the css would be broken once the user implemented a tab panel for secondary nav.

I think it's important to show how tab contents would be defined, but am interested in other opinions on this.
@priley86 @jeff-phillips-18 What do you think?

If we do update the secondary navigation example to show tab contents, and the css no longer renders styles as they should, then I would suggest we merge this PR (with the tab contents and broken CSS*) and open an issue against patternfly core.

*Just wanted to note that the CSS really isn't that bad in the original implementation. There are just a few missing styles but the component is still functional.

[jeff-phillips-18]

Agreed @jgiardino. IMO the example should show the typical use of the tab component and the secondary tab components. I would rather see this mimic the Patternfly layout and examples as closely as possible. If there are then issues due to the Patternfly code base, those issues can/should be addressed there.

[priley86]

yep - fine with this resolution. I think an upstream issue in Core is warranted if we can't address that here. We can always circle back later if need be...

[priley86]

also @dabeng @jgiardino - we are using React 16.2 now, so fragments should be fine to address your earlier point...

[dabeng]

Hi @priley86 , I have rebase this PR , but when I introduced Fragment into tabs story as shown below:

import React,{ Fragment } from 'react';

    <Fragment>
      <p>
        <b>Note:</b>
        This variation is not keyboard accessible and is not recommended for use
        where accessibility is a priority.
      </p>
      <TabContainer id="tabs-with-dropdown" defaultActiveKey="first">
       ...
      </TabContainer>
    </Fragment>

I always got the following warnings:

[jeff-phillips-18]

You don't need the <Fragment> wrapper but you do need to add a key tag on the children:

  return [
    <p key="note">
      <b>Note:</b>
      This variation is not keyboard accessible and is not recommended for use
      where accessibility is a priority.
    </p>,
    <TabContainer id="tabs-with-dropdown" defaultActiveKey="first" key="container">
     ...
    </TabContainer>
  ];

[jgiardino]

@dabeng After further discussion with @LHinson and @mcarrano, there's one more change we'd like to request. It wasn't obvious during our discussion that this component supported the .nav-tabs-pf styles until I pointed out that it's a setting under Knobs.

Can we split out the tabs examples into the following list items in the Storybook navigation?:

Tabs

• Basic Tabs
  • Remove the KNOB Navigation Style
  • Only show these tabs with the "tabs" option using the .nav-tabs style
• Basic Tabs - PF style
  • Remove the KNOB Navigation Style
  • Only show these tabs with the "patternfly style" option using the .nav-tabs-pf style
• Tabs with Dropdown
  • Remove the KNOB Navigation Style
  • Only show these tabs with the "tabs" option using the .nav-tabs style
• Tabs with Dropdown - PF style
  • Remove the KNOB Navigation Style
  • Only show these tabs with the "patternfly style" option using the .nav-tabs-pf style
• Secondary Tabs
  • no change

[jgiardino]