Skip to content

Tabs #1

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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

Tabs #1

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21b8dd2
Update RFC_Technical_Design_Document.md
Apatil15 Oct 18, 2021
f792905
fix(rfc tabs): fixed as discussed suggestions
Apatil15 Oct 21, 2021
281c15a
fix(tabs) : addressed all the review comments
Apatil15 Oct 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 69 additions & 44 deletions RFC_Technical_Design_Document.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,46 @@


# RFC : Technical design document

## Component - Tab

### Scope of the Document

This document outlines the technical design document of Tab component functionalities. Its highlights the component design, properties, and initial implementation.

### Component Overview

Tabs organize and allow users to move between content without having to navigate away from a page. The Tabs component organizes multiple sections of related content so a user can view one section at a time.

### Component Design

> Component designed based on the scope of the tab component, by default, the Tab simply renders data passed to it.
### Scope
- Tab Container: Tab container is a component that manages the context and state of the tabs.
- Tab List: Wrapper for the tab components.
- Tab: Element that serves as a label for one of the tab panels and can be activated to display that panel.
- Tab Panel: Is a presentational component which accepts a tab’s content as its children.

#### Tab Container
The component or element the *Tabs* should render as *Fragment*.
### Scope

- Tab Container: Tab container is a component that manages the context and state of the tabs.
- Tab List: Wrapper for the tab components.
- Tab: Element that serves as a label for one of the tab panels and can be activated to display that panel.
- Tab Panel: Is a presentational component which accepts a tab’s content as its children.

#### Tabs

The component or element the *Tabs* should render as *Fragment*.
Copy link

@dmedjordan dmedjordan Oct 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Provides context and state.

##### Properties

| Props | Type | Default | Required | Description |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

| :------------- |:-------------:|:-------------:|:-------------:|:-------------|
| ClassName| Object | | | Override or extend component style.|
| Id | String | | | Unique identification of tabs. |

#### TabList

The component or element the *TabList* should render as *div*.

##### Properties

| Props | Type | Default | Required | Description |
Copy link
Contributor

@sh0ji sh0ji Nov 3, 2021 •
edited by Apatil15
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

| :------------- |:-------------:|:-------------:|:-------------:|:-------------|
| ClassName| Object | | | Override or extend component style.|
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Expand All @@ -45,7 +55,9 @@ The component or element the *TabList* should render as *div*.

The component or element the ‘**_Tab’_** should render as ‘**_button’._**
Children of Tablist component, display based on label, by default index is 0

##### Properties

| Props | Type | Default | Required | Description |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

| :------------- |:-------------:|:-------------:|:-------------:|:-------------|
| Active| Boolean | | | Selected, Unselected flag, based on this flag active tab.|
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Apatil15 , were any of these bullets addressed?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Specification document lists the States as:

  • Selected
  • Unselected
  • Disabled

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Expand All @@ -55,76 +67,87 @@ Children of Tablist component, display based on label, by default index is 0
| ClassName | Object | | =Orientation| Override or extend component style. |

#### TabPanel

The component or element the ‘**_TabPanel’_** should render as ‘**_div’._**

element that contains the content associated with a tab

##### Properties

| Props | Type | Default | Required | Description |
| :------------- |:-------------:|:-------------:|:-------------:|:-------------|
| ClassName| Object | | | Override or extend component style.|
| Id | String | | | Unique identification of tabs. |
| Children | Node | | | Override or extend component style. |
| Id | String | | | Unique identification of tabpanel. |
| Children | Node | | | Any component or element. |

### Design Flow
<img width="578" alt="image" src="https://user-images.githubusercontent.com/25932161/137789481-c6078525-413c-47b8-b520-498934b99f6b.png">
### Design Flow

<img width="596" alt="image" src="https://user-images.githubusercontent.com/25932161/138617008-1f0f2e72-e5da-4d7c-882d-496c9ca21635.png">

### Component Style

#### Mixings

- nds—tabs (base)
- nds-tabs__tablist
- nds-tabs__tabpanel
- nds—tabs__tab
- nds—tabs (base)
- nds-tabs__tablist
- nds-tabs__tabpanel
- nds—tabs__tab
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SharePoint version did not have nds-tabs_tab listed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


### Keyboard Intraction
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


| Key | Action|
| :------------- |:-------------|
| ArrowLeft| Move focus to previous column. |
| ArrowRight| Move focus to next column. |
|Tab | Focus moves into the tablist from the place of active tab.
|Space or Enter | Activate tab if tabs are not activated.
|Home | Focus move to first tab.
|End | Focus moves to last tab.
| ArrowLeft| Move focus to previous tab. |
| ArrowRight| Move focus to next tab. |
|Tab | Focus moves into the tablist from the place of active tab.
|Space or Enter | Activate tab if tabs are not activated.
|Home | Focus move to first tab.
|End | Focus moves to last tab.

## Accessibility
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...


### ARIA Roles

#### Tab

| Role | Description|
| :------------- |:-------------|
| role=”tab”| Indicate that is tab. |
| aria-selected| Set for active tab. |
|aria-control | Set associate idb.
| role=”tab”| Indicate that is tab. |
| aria-selected| Set for active tab. |
| aria-control | Set associate id.

#### TabList

| Role | Description|
| :------------- |:-------------|
| role=”tablist”| Indicate that is tablist. |
| aria-orientation| Indicate horizontally or vertically tab. |
| role=”tablist”| Indicate that is tablist. |
| aria-orientation| Indicate horizontally or vertically tab. |

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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 missed this,
aria-labelledby vs aria-label
https://developers.google.com/web/fundamentals/accessibility/semantics-aria/aria-labels-and-relationships

## Implementation example
#### TabPanel

| Role | Description|
| :------------- |:-------------|
| role=”tabpanel”| Indicate that is tabpanel. |
| aria-label| may be used on any element, not just labelable elements. |

## Implementation example
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


<Tabs index={tabIndex} onSelect={handleTabsChange}>
<TabList>
<Tab>One</Tab>
<Tab>Two</Tab>
<Tab>Three</Tab>
</TabList>
<TabPanel>
<span>This is tab one</span>
</TabPanel>
<TabPanel>
<p>This is tab two</p>
</TabPanel>
<TabPanel>
<div>This is tab three. </div>
</TabPanel>
</Tabs>
<TabList>
<Tab>One</Tab>
<Tab>Two</Tab>
<Tab>Three</Tab>
</TabList>
<TabPanel>
<span>This is tab one</span>
</TabPanel>
<TabPanel>
<p>This is tab two</p>
</TabPanel>
<TabPanel>
<div>This is tab three. </div>
</TabPanel>
</Tabs>

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the Rendering example not needed?

## Component render example

Expand All @@ -146,7 +169,9 @@ element that contains the content associated with a tab
</ Fragment >

### Anatomy document

[https://docs.google.com/document/d/1D0MjvCYdCaaHDJGYcPk8u9pSREkVpAac8JqNYzviHc0/edit#heading=h.5old1136lvpw](https://docs.google.com/document/d/1D0MjvCYdCaaHDJGYcPk8u9pSREkVpAac8JqNYzviHc0/edit#heading=h.5old1136lvpw)

### Refs
https://www.w3.org/TR/wai-aria-practices-1.1/examples/tabs/tabs-1/tabs.html

<https://www.w3.org/TR/wai-aria-practices-1.1/examples/tabs/tabs-1/tabs.html>