Tabs

[Apatil15]

Please add your suggestions for the RFC Technical design document

[sh0ji]

Summarized comment

[dmedjordan]

The version on SharePoint has Tabs rendered as 'div'. Evan presented rational for both. Just wanted to make sure 'Fragment' is where the final decision landed.

[Apatil15]

Tabs is just a wrapper on tab list and tab panel, without any events, In case add any events or pros it would be carry forward to the child element like tab list.

[dmedjordan]

Evan had a comment about the Required field in the SharePoint version. You responded but I am not clear on what the end result is supposed to be.

[sh0ji]

The result should be that all rows need either true or false in the required column. The design should be as unambiguous as possible, so let's not leave anything up to inference like "empty means false."

[dmedjordan]

Based on Evan's comment from Friday, each cell in the Required column (in each section) needs to have a value of Yes (True) or No (False)

[dmedjordan]

The below are observations...not necessarily anything wrong

• Name and Description for line #48 is different from SharePoint version
• Line #50 is not in the SharePoint version
• Is the Description for line #52 supposed to be the same as the Description for line #51?

[Apatil15]

• Line #48 I changed tabs is a '** Fragment**' and fragment has no events or props. I shifted events and props in tablist.

• Line #50 I went through following document for aria-orientation. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques/Using_the_aria-orientation_attribute. default orientation would be "Horizontal" and as mentioned passing "Orientation" props to aria-orientation.

• Line #50 is the property by default number value is 0 and Line #51 is a event and when user call by default return the event 0 index from the component

[dmedjordan]

Not sure if my last question was answered. Are the Descriptions for SelectedIndex (line #51) and onSelected (line #52) supposed to be the same?

[sh0ji]

Right, the onSelected props should be something like, "A callback function that is triggered when a tab is selected."

[dmedjordan]

More observations...not necessarily anything wrong

• In the SharePoint version, the Name for line #63 is Selected
• In the SharePoint version, the Description for line # 64 does NOT have the '<![endif]-->'
• In the SharePoint version, there is a Name / Description for Label

[dmedjordan]

@Apatil15 , were any of these bullets addressed?

[dmedjordan]

The Specification document lists the States as:

• Selected
• Unselected
• Disabled

[sh0ji]

Right, Active should be selected and its description should probably call out that it sets aria-selected on the tab. The current description is a bit confusing. Maybe, "When set, indicates that the tab is currently selected and its corresponding TabPanel is visible. Sets the aria-selected attribute."

[dmedjordan]

In the SharePoint version, the Description references 'tabpanel'

[Apatil15]

Yes, you are right it should be tabpanel.

[dmedjordan]

In the SharePoint version, the Description reads "Any component or element"

[Apatil15]

Done

[dmedjordan]

Noted the following in the Design Flow diagrams:

• <> Tabs: SharePoint version had an onSelected item
• <> TabList: SharePoint version did not have an onSelected item
• <> Tab: Should the Icon and IconRight items be listed?

[Apatil15]

You are right, It should not be there, I changed it

[dmedjordan]

In the Interface-Tab design flow:

• Should Index be labeled as defaultIndex?
• Should ClassName be listed?

[dmedjordan]

The SharePoint version did not have nds-tabs_tab listed

[sh0ji]

This looks like it's in response to my point about following BEM conventions to encode the anatomy.

• "tabs" is the block name
• tablist, tab, and tabpanel are the elements
  • this leads to the awkward nds-tabs__tab
• an example of a modifier might be something like nds-tabs--vertical if we wanted a different display for a vertical orientation (using aria-orientation as a selector would probably be more semantic, though).

[dmedjordan]

The SharePoint version states "previous tab" instead of "previous column"
NOTE: If the tab is vertical, should we list ArrowUp to move focus to the previous tab?

[dmedjordan]

I didn't see any mention of ArrowUp or ArrowDown.

[dmedjordan]

The SharePoint version states "next tab" instead of "next column"
NOTE: If the tab is vertical, should we list ArrowDown to move focus to the next tab?

[sh0ji]

Great points! Yes, if we're supporting aria-orientation somehow, I like the idea of using ArrowUp/ArrowDown when aria-orientation="vertical".

[dmedjordan]

"idb" should be "id"

[dmedjordan]

In the SharePoint version, there was a section for TabPanel:

[Apatil15]

I missed this,
aria-labelledby vs aria-label
https://developers.google.com/web/fundamentals/accessibility/semantics-aria/aria-labels-and-relationships

[dmedjordan]

There is a Rendering example in the SharePoint version (appears to be the same as the Implementation example)

[dmedjordan]

Was the Rendering example not needed?

[Apatil15]

@dmedjordan Thank you, I have addressed all the given suggestions

[sh0ji]

Thanks for being the brave first RFC, @Apatil15!

Like my general comment in #2, it's not 100% clear to me from this what the API is you're proposing. For instance, should I be able to write code like this and expect it to work?

<TabList>
  <Tab selected aria-controls="panel-animals">Animals</Tab>
  <Tab aria-controls="panel-plants">Plants</Tab>
</TabList>
<p>A persistent paragraph between the tabs and their panels</p>
<TabPanel id="panel-animals">...info about animals...</TabPanel>
<TabPanel id="panel-plants">...info about plants...</TabPanel>

Or what happens if I do this?

<Tabs>
  <TabList selectedIndex={1}>
    <Tab defaultIndex={1} aria-controls="panel-animals">Animals</Tab>
    <Tab disabled aria-controls="panel-plants">Plants</Tab>
  </TabList>
  <TabPanel id="panel-animals">...info about animals...</TabPanel>
  <TabPanel id="panel-plants">...info about plants...</TabPanel>
</Tabs>

I mentioned it in a few comments, but examples like this would go a long way to helping readers understand your design.
@erinoh
#1 Example with selected props not work. We haven't selected property, but on selected event maintaining selected index and based this index selecting a tab.
#2 selectedIndex always override the default index.

[sh0ji]

This is great in general, thanks for including it! I do think it would be even more effective if required attributes like role or aria-selected were documented under the relevant component rather than in a separate "accessibility" section.

Perhaps each component should have an "Attributes" section that would include attributes that are required for accessibility, but also potentially for other relevant attributes that you'd use to accomplish core functionality like the the disabled HTML attribute to disable a tab...

[sh0ji]

Should be aria-controls, and the description could use some work. It would also be great if you linked to the definition in the specification for attributes like this too.

Here's my suggestion, which is more or less the actual specification description for this attribute but specific to the Tabs design you've proposed.

Suggested change

	|aria-control | Set associate idb.
	| [aria-controls](https://www.w3.org/TR/wai-aria/#aria-controls) | Identifies the `TabPanel` that's being controlled by this `Tab`.

[sh0ji]

Right, the onSelected props should be something like, "A callback function that is triggered when a tab is selected."

[sh0ji]

I think the diff is off because you accidentally pushed this to main before a branch, so it's not letting me comment on the individual lines. Here are my thoughts about each prop:

• ClassName - this is presumably already part of the interface that TabListProps extends (React.HTMLAttributes<HTMLDivElement>, maybe? I don't actually know for sure...). If it is part of the extended interface, you don't need to call it out here unless you're changing its functionality in some way.
• Id - same comment as ClassName.
• Orientation - props always have to start with a lowercase, so this would be orientation. How's it different from aria-orientation listed right below?
• SelectedIndex - same comment on casing. What's the benefit of using TabList to control the selected Tab over setting the state on the Tab itself with something like <Tab selected />? If this is a "controlled mode" for the component, you should call that out the fact that it can be controlled or uncontrolled in the design overview above.
• onSelected - This seems like a very useful prop to me 👍 (it does need a better description, as @dmedjordan pointed out)

@erinoh
aria-orientation : This prop using for accessibility and this props work behind the scene once set orientation prop.

[sh0ji]

Right, Active should be selected and its description should probably call out that it sets aria-selected on the tab. The current description is a bit confusing. Maybe, "When set, indicates that the tab is currently selected and its corresponding TabPanel is visible. Sets the aria-selected attribute."

[sh0ji]

In addition to the naming and description comment that @dmedjordan made, could you clarify how this works in conjunction with selectedIndex on the TabList? What if I did this?

<TabList selectedIndex={1}>
  <Tab selected>Animals</Tab>  // index={0}
  <Tab>Plants</Tab>            // index={1}
</Tab>

Which way of setting the selected tab wins?

[sh0ji]

I can't comment on some of the other props, so here's my feedback:

• Disabled - this should be inherited by the Tab's underlying interface if possible. In other words, it should just be the disabled HTML attribute, not a new prop that's part of this design. You should call out how you're leveraging existing HTML functionality either in the component summary or in a separate section if you're leveraging a lot of it.
• defaultIndex - This one's a bit confusing to me. Does it allow you to change the order of the tabs without changing the structural order of the tabs? In other words, would the following be true?

// this JSX:
<Tab defaultIndex={1}>Animals</Tab>
<Tab defaultIndex={0}>Plants</Tab>

// renders to this HTML:
<button role="tab">Plants</button>
<button role="tab">Animals</button>

And if so, could you help me understand why that's necessary?

[sh0ji]

I really like this being included here since it's part of what will need to be implemented, but some of this is currently being articulated in the functional design acceptance criteria, isn't it, @dmedjordan? Would you want to merge the two or just use the functional design Jira ticket as the source of truth for expected interaction behaviors like this?

[sh0ji]

I'd love to see more smaller examples interspersed throughout your design rather than one big one at the end. For instance, under Tab, you could have an example like this:

<Tab selected>Animals</Tab>

// should render to this HTML
<button class="nds-tab" role="tab" aria-selected="true">Animals</button>

I don't know if that's actually accurate to your design, but you hopefully get the idea...smaller examples that tell more of a story.

[sh0ji]

Our Tabs design was accepted in #5 so I'm closing this PR.