From 5132ca769b47637ba9980d3c1072050eece11eed Mon Sep 17 00:00:00 2001 From: JRebella Date: Thu, 14 Apr 2022 14:12:38 -0300 Subject: [PATCH 1/4] chore(tabs): initial documentation with API questions --- accepted/0003-component-tabs.md | 112 ++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 accepted/0003-component-tabs.md diff --git a/accepted/0003-component-tabs.md b/accepted/0003-component-tabs.md new file mode 100644 index 0000000..f05ed87 --- /dev/null +++ b/accepted/0003-component-tabs.md @@ -0,0 +1,112 @@ +- Start Date: 2022/04/11 +- Related documents: + - [acceptance criteria](https://wwnorton.atlassian.net/browse/NDS-235) + - [documentation draft](https://docs.google.com/document/d/1D0MjvCYdCaaHDJGYcPk8u9pSREkVpAac8JqNYzviHc0) + - [visual design](@TODO) + - [names, states, and properties](@TODO) +- RFC PR: @TODO + +## Detailed design + +### API option 1: tab content as children and reflected upwards the tree using portals + +#### Internally controlled (Uncontrolled) + +```tsx + + + Cats content + + + Dogs content + + + Horses content + + +``` + +#### Externally controlled + +```tsx + + + Cats content + + + Dogs content + + + Horses content + + +``` + +- Simpler API +- More complex implementation? The panel content is passed as children to the Tab component but it can be placed at a higher level in the DOM tree using Portals + +### API option 2: a specific "TabPanel" component, only related to the Tab component by an ID + +```tsx +<> + + Cats + Dogs + Horses + + + Cats content + Dogs content + Horses content + +``` + +- One more component TabPanel +- I can't see how an uncontrolled/unmanaged version could be implemented, current active tab status must be handled by the parent. Mui uses this API and they don't offer an uncontrolled version (they offer uncontrolled versions for most of their components) +- More flexible API, the Tabs component is so detached that could be used differently like having the tab selector below the content + +### Simple example of final rendered HTML + +```tsx +
+ + + + +
+
+ Cats content +
+ + +
+
+``` From 14330ab8af0cee9cf57f219d5473b77bd3bb29fc Mon Sep 17 00:00:00 2001 From: JRebella Date: Thu, 14 Apr 2022 14:34:41 -0300 Subject: [PATCH 2/4] chore(tabs): added more discussion points --- accepted/0003-component-tabs.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/accepted/0003-component-tabs.md b/accepted/0003-component-tabs.md index f05ed87..76805fc 100644 --- a/accepted/0003-component-tabs.md +++ b/accepted/0003-component-tabs.md @@ -44,6 +44,8 @@ - Simpler API - More complex implementation? The panel content is passed as children to the Tab component but it can be placed at a higher level in the DOM tree using Portals +- Used by ant-design https://codesandbox.io/s/dmkw8l +- Since the label is a string prop, adding icons is not as intuitive and flexible as having the label be the children. We could use the same API used in the button component for handling icon and icon position (`icon`, `iconRight`, `iconOnly` as props for the `Tab` component) ### API option 2: a specific "TabPanel" component, only related to the Tab component by an ID @@ -62,7 +64,8 @@ ``` - One more component TabPanel -- I can't see how an uncontrolled/unmanaged version could be implemented, current active tab status must be handled by the parent. Mui uses this API and they don't offer an uncontrolled version (they offer uncontrolled versions for most of their components) +- I can't see how an uncontrolled/unmanaged version could be easily implemented without mayor changes, current active tab state must be handled by the parent. Could be done by adding a wrapper component that could distribute the state data downwards maybe using the context API or by using cloneElement to inject state props (I prefer staying away from cloneElement if possible) +- Mui uses this API and they don't offer an uncontrolled version (they offer uncontrolled versions for most of their components) - More flexible API, the Tabs component is so detached that could be used differently like having the tab selector below the content ### Simple example of final rendered HTML From 169814b1a15d4bf6a158fd946e06151f33a6bab8 Mon Sep 17 00:00:00 2001 From: JRebella Date: Mon, 18 Apr 2022 13:52:13 -0300 Subject: [PATCH 3/4] chore(tabs): updated RFC, added detailed design --- accepted/0003-component-tabs.md | 158 +++++++++++++++++++++----------- 1 file changed, 105 insertions(+), 53 deletions(-) diff --git a/accepted/0003-component-tabs.md b/accepted/0003-component-tabs.md index 76805fc..bd3215b 100644 --- a/accepted/0003-component-tabs.md +++ b/accepted/0003-component-tabs.md @@ -3,71 +3,96 @@ - [acceptance criteria](https://wwnorton.atlassian.net/browse/NDS-235) - [documentation draft](https://docs.google.com/document/d/1D0MjvCYdCaaHDJGYcPk8u9pSREkVpAac8JqNYzviHc0) - [visual design](@TODO) - - [names, states, and properties](@TODO) -- RFC PR: @TODO +- [RFC PR](https://github.com/wwnorton/design-system-rfcs/pull/5) + +## Summary + +`Tabs` allow the user to select layered sections of content to display within the same space. + +A list of tab headers is displayed using the `TabList` and `Tab` components, allowing the user to interact and select the tab content they want to see. The contents of each tab is contained in a `TabPanel` component, all wrapped within a `TabPanels` container. + +### Sub components + +- `` - The highest-order wrapper that contains all the other sub-components. Handles data management and distribution across it's children +- `` - Container for the `` components +- `` - An interactive button within the `TabList`, allows the user to navigate between the different sections of content +- `` - Container for the `` components +- `` - Container for the content of each section, contained within `TabPanels` ## Detailed design -### API option 1: tab content as children and reflected upwards the tree using portals +A stateful component with both uncontrolled and controlled versions (state managed internally / state managed externally). -#### Internally controlled (Uncontrolled) +The only state managed by this component is the currently active section of content, identified by its index. `Tab` components must be in order with its corresponding `TabPanel`s in order to assure that the `Tab` buttons activate the correct section of content -```tsx - - - Cats content - - - Dogs content - - - Horses content - - -``` +### Tabs + +`` extends the `React.ComponentPropsWithRef<'div'>` interface and adds the following props: + +| Name | Type | Description | Required | Default | +| -------------- | ------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------ | -------- | ----------- | +| `value` | `number` | The currently active tab, for controlled use only | `false` | `undefined` | +| `onChange` | `(payload: { value: number; contents: ReactNode; }) => void` | Callback for when the user interacts with one of the `Tab` buttons | `false` | 0 | +| `initialValue` | `number` | Sets the default value for the currently active tab, for uncontrolled use only. Will be ignored if the `value` prop is defined | `false` | `undefined` | + +### TabList + +`` extends the `React.ComponentPropsWithRef<'div'>` interface with a `role="tablist"` attribute. + +### Tab + +`` extends the `React.ComponentPropsWithRef<'button'>` interface with `role="tab"`, `aria-selected="true/false"` and `aria-controls="tab-ID"` (an autogenerated unique prefix might be needed to support the case were there are multiple tab components rendered at the same time and share tab names) attributes. The values for `aria-selected` and `aria-controls` should be calculated using the state delivered by the `Tabs` wrapper + +| Name | Type | Description | Required | Default | +| ------- | ----------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | ----------- | +| `value` | `React.Key` | An unique key to identify this `Tab`, must be the same as the value provided to its corresponding `TabPanel`. If unset, the children position index will be used | `false` | `undefined` | + +### TabPanels + +`` extends the `React.ComponentPropsWithRef<'div'>` interface. -#### Externally controlled +### TabPanel + +`` extends the `React.ComponentPropsWithRef<'div'>` interface with `role="tabpanel"` and an `id` attributes. + +| Name | Type | Description | Required | Default | +| ------- | ----------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | ----------- | +| `value` | `React.Key` | An unique key to identify this `TabPanel`, must be the same as the value provided to its corresponding `Tab`. If unset, the children position index will be used | `false` | `undefined` | + +### Simple Usage - Uncontrolled/State managed internally ```tsx - - - Cats content - - - Dogs content - - - Horses content - + + + Cats + Dogs + Horses + + + Cats content + Dogs content + Horses content + ``` -- Simpler API -- More complex implementation? The panel content is passed as children to the Tab component but it can be placed at a higher level in the DOM tree using Portals -- Used by ant-design https://codesandbox.io/s/dmkw8l -- Since the label is a string prop, adding icons is not as intuitive and flexible as having the label be the children. We could use the same API used in the button component for handling icon and icon position (`icon`, `iconRight`, `iconOnly` as props for the `Tab` component) - -### API option 2: a specific "TabPanel" component, only related to the Tab component by an ID +### Simple Usage - Controlled/State managed externally ```tsx -<> - - Cats - Dogs - Horses - - - Cats content - Dogs content - Horses content - + + + Cats + Dogs + Horses + + + Cats content + Dogs content + Horses content + + ``` -- One more component TabPanel -- I can't see how an uncontrolled/unmanaged version could be easily implemented without mayor changes, current active tab state must be handled by the parent. Could be done by adding a wrapper component that could distribute the state data downwards maybe using the context API or by using cloneElement to inject state props (I prefer staying away from cloneElement if possible) -- Mui uses this API and they don't offer an uncontrolled version (they offer uncontrolled versions for most of their components) -- More flexible API, the Tabs component is so detached that could be used differently like having the tab selector below the content - ### Simple example of final rendered HTML ```tsx @@ -101,15 +126,42 @@
-
+
Cats content
- ``` + +## Alternatives + +Ant-design uses a simpler and more minimalistic API, which combines the `Tab` and `TabPanel` components into one and only a single `Tabs` wrapper. A clear disadvantage of this API is that it doesn't reflect the structure of the actual rendered HTML. Also it requires a more complex implementation due to the need to use Portals in order to elevate the tab content above its container in the final DOM. + +```tsx + + + Content of Tab Pane 1 + + + Content of Tab Pane 2 + + + Content of Tab Pane 3 + + +``` + +## Unresolved questions + +- We need a way to identify each `Tab` and relate it to its corresponding `TabPanel`. This is needed to keep track of currently active tab and setting the `aria-controls` attribute + +We can: + +1. Force the user to provide a `value: React.Key` prop to each `Tab` and `TabPanel` (expecting the key to be equal for corresponding tab/tabPanel pairs) and use that +2. Make it optional, and when none is provided, fallback to the children position index. Both the `TabList` and `TabPanels` parents will most likely need to iterate over its children props and check whether they have a `value` prop or not and inject its corresponding index as `value` if needed From 76d25f3a7ec879fa8eddce940a9a65b832f58bd1 Mon Sep 17 00:00:00 2001 From: JRebella Date: Thu, 21 Apr 2022 13:21:34 -0300 Subject: [PATCH 4/4] chore(tabs): reworked props --- accepted/0003-component-tabs.md | 76 +++++++++++++++++++-------------- 1 file changed, 45 insertions(+), 31 deletions(-) diff --git a/accepted/0003-component-tabs.md b/accepted/0003-component-tabs.md index bd3215b..bbc647a 100644 --- a/accepted/0003-component-tabs.md +++ b/accepted/0003-component-tabs.md @@ -11,6 +11,13 @@ A list of tab headers is displayed using the `TabList` and `Tab` components, allowing the user to interact and select the tab content they want to see. The contents of each tab is contained in a `TabPanel` component, all wrapped within a `TabPanels` container. +## Definitions + +- `Controlled/uncontrolled or managed/unmanaged state`: The only state this component is interested in is which tab is the currently selected one. The controlled/uncontrolled distinction is about _who_ is responsible for managing this state. + - Controlled means that the parent (the component invoking the `Tabs` component) is responsible of managing this state, this way the user can **control the state however they want**. In order to use the component in controlled mode, the user must provide `selectedIndex` and `onChange` props to the `Tabs` component. + - Uncontrolled means that the state is managed internally by the `Tabs` component, the parent does not need to provide any specific props and **can't control the state**. In uncontrolled mode, the only thing that the parent can control is the default state for the initial render, this is done through the optional `defaultSelectedIndex` prop. +- `Position index`: this refers to the position of a component relative to their parent. If they are the first child their position index would be 0, if they are the second it would be 1 and so on. + ### Sub components - `` - The highest-order wrapper that contains all the other sub-components. Handles data management and distribution across it's children @@ -23,46 +30,52 @@ A list of tab headers is displayed using the `TabList` and `Tab` components, all A stateful component with both uncontrolled and controlled versions (state managed internally / state managed externally). -The only state managed by this component is the currently active section of content, identified by its index. `Tab` components must be in order with its corresponding `TabPanel`s in order to assure that the `Tab` buttons activate the correct section of content +The only state needed by this component is the currently active section of content, identified by its index. `Tab` components must be in order with its corresponding `TabPanel`s in order to assure that the `Tab` buttons activate the correct section of content ### Tabs `` extends the `React.ComponentPropsWithRef<'div'>` interface and adds the following props: -| Name | Type | Description | Required | Default | -| -------------- | ------------------------------------------------------------ | ------------------------------------------------------------------------------------------------------------------------------ | -------- | ----------- | -| `value` | `number` | The currently active tab, for controlled use only | `false` | `undefined` | -| `onChange` | `(payload: { value: number; contents: ReactNode; }) => void` | Callback for when the user interacts with one of the `Tab` buttons | `false` | 0 | -| `initialValue` | `number` | Sets the default value for the currently active tab, for uncontrolled use only. Will be ignored if the `value` prop is defined | `false` | `undefined` | +| Name | Type | Description | Required | Default | +| ---------------------- | --------------------------------- | -------------------------------------------------------------------------------------------------------------------- | -------- | ----------- | +| `selectedIndex` | `number` | The currently active tab, for controlled use only | `false` | `undefined` | +| `onChange` | `(selectedIndex: number) => void` | Callback for when the user interacts with one of the `Tab` buttons, will pass the position index of the selected tab | `false` | `undefined` | +| `defaultSelectedIndex` | `number` | Sets the default active tab, for uncontrolled use only. Will be ignored if the `selectedIndex` prop is defined | `false` | 0 | ### TabList -`` extends the `React.ComponentPropsWithRef<'div'>` interface with a `role="tablist"` attribute. +`` extends the `React.ComponentPropsWithRef<'div'>` interface with a `role="tablist"` attribute. A simple wrapper for the `Tab` components. ### Tab -`` extends the `React.ComponentPropsWithRef<'button'>` interface with `role="tab"`, `aria-selected="true/false"` and `aria-controls="tab-ID"` (an autogenerated unique prefix might be needed to support the case were there are multiple tab components rendered at the same time and share tab names) attributes. The values for `aria-selected` and `aria-controls` should be calculated using the state delivered by the `Tabs` wrapper +`` extends the `React.ComponentPropsWithRef<'button'>` interface. -| Name | Type | Description | Required | Default | -| ------- | ----------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | ----------- | -| `value` | `React.Key` | An unique key to identify this `Tab`, must be the same as the value provided to its corresponding `TabPanel`. If unset, the children position index will be used | `false` | `undefined` | +The `Tab` component is responsible for rendering a `button` with the following (auto generated, the user does not need to provide these through props) attributes: + +- `type="button"` +- `role="tab"` +- `aria-selected`: `true` if this `Tab` refers to the currently active tab panel, else `false` +- `aria-controls`: The value must be the `id` of the corresponding `TabPanel` this `Tab` controls. For example `tab-0` if this is the `Tab` with position index 0 +- `id`: The value must be unique and will be referred to in the corresponding `TabPanel` through the `aria-labelledby` attribute. For example `tab-header-0` if this is the `Tab` with position index 0 ### TabPanels -`` extends the `React.ComponentPropsWithRef<'div'>` interface. +`` extends the `React.ComponentPropsWithRef<'div'>` interface. A simple wrapper for the `TabPanel` components. ### TabPanel -`` extends the `React.ComponentPropsWithRef<'div'>` interface with `role="tabpanel"` and an `id` attributes. +`` extends the `React.ComponentPropsWithRef<'div'>` interface with `role="tabpanel"` and an autogenerated `id` attributes. + +The `TabPanel` component is responsible for rendering a `div` with the following (auto generated, the user does not need to provide these through props) attributes: -| Name | Type | Description | Required | Default | -| ------- | ----------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------- | ----------- | -| `value` | `React.Key` | An unique key to identify this `TabPanel`, must be the same as the value provided to its corresponding `Tab`. If unset, the children position index will be used | `false` | `undefined` | +- `role="tabpanel"` +- `aria-labelledby`: The value must be the `id` of the corresponding `Tab` this `TabPanel` is controlled by. For example `tab-header-0` if this is the `TabPanel` with position index 0 +- `id`: The value must be unique and will be referred to in the corresponding `Tab` through the `aria-controls` attribute. For example `tab-0` if this is the `TabPanel` with position index 0 ### Simple Usage - Uncontrolled/State managed internally ```tsx - + Cats Dogs @@ -79,7 +92,7 @@ The only state managed by this component is the currently active section of cont ### Simple Usage - Controlled/State managed externally ```tsx - + Cats Dogs @@ -96,13 +109,13 @@ The only state managed by this component is the currently active section of cont ### Simple example of final rendered HTML ```tsx -
+
@@ -110,8 +123,8 @@ The only state managed by this component is the currently active section of cont type="button" role="tab" aria-selected="false" - aria-controls="dogs-tab" - id="dogs" + aria-controls="tab-1" + id="tab-header-1" > Dogs @@ -119,20 +132,20 @@ The only state managed by this component is the currently active section of cont type="button" role="tab" aria-selected="false" - aria-controls="horses-tab" - id="horses" + aria-controls="tab-2" + id="tab-header-2" > Horses
-
+
Cats content
-