Skip to content
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

Reduce Tabs component re-renders and fix controlled component functionality #31

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Reduce Tabs component re-renders and fix controlled component functionality #31

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
48a3a07
Running `npm install` updated some package-lock info
coreyafunk Sep 2, 2021
e943864
Separate the Tab component and use `useCallback` to reduce unnecessar…
coreyafunk Sep 2, 2021
2fd7bbf
Add checkbox to demonstrate controlled tab functionality
coreyafunk Sep 2, 2021
618f34e
Clean up
coreyafunk Sep 2, 2021
31e89c8
Simplify state around active/default tab props
coreyafunk Sep 2, 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
34 changes: 24 additions & 10 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

105 changes: 65 additions & 40 deletions src/core/components/tabs/Tabs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,42 @@
import React, { useEffect, useState } from 'react'
import React, { useEffect, useState, useCallback } from 'react'
import PropTypes from 'prop-types'
import classNames from 'classnames'
import { KEYCODES } from '../../constants/constants'
import ThemeWrapper from '../../utils/ThemeWrapper'

const Tab = ({ styling, isActive, item, onTabChange, ...rest }) => {
const tabStyle = classNames(
styling.tab,
isActive && styling.active,
!item.enabled && styling.disabled
)

const handleClick = useCallback(() => {
onTabChange(item)
}, [onTabChange])

const handleKeyPress = useCallback(
event => {
if ([KEYCODES.SPACE, KEYCODES.ENTER].includes(event.charCode)) {
event.preventDefault()
onTabChange(item)
}
},
[onTabChange]
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't item also be included in the dependencies here? just in case it were to change?

)

return (
<li
tabIndex={0}
className={tabStyle}
onClick={handleClick}
onKeyPress={handleKeyPress}
{...rest}>
{item.label}
</li>
)
}

export const Tabs = ({
activeTab,
className = '',
Expand All @@ -13,55 +46,47 @@ export const Tabs = ({
styling,
...rest
}) => {
// The initial selected tab is either `activeTab`, `defaultActiveTab`, or undefined (unset), evaluated in that order
const [selectedTab, setSelectedTab] = useState(activeTab || defaultActiveTab)

/* When `activeTab` prop changes, update the selected tab */
useEffect(() => {
const selectedTab = options.find(i => i.label === (activeTab || defaultActiveTab))
if (selectedTab) {
setSelectedTab(selectedTab.label)
if (activeTab) {
setSelectedTab(activeTab)
}
}, [activeTab, defaultActiveTab, options])
}, [activeTab])

const containerStyle = classNames(className, styling.container)

const handleKeyPress = (e, item) => {
if ([KEYCODES.SPACE, KEYCODES.ENTER].includes(e.charCode)) {
e.preventDefault()
setSelectedTab(item)
}
}

const Tab = ({ item }) => {
const tabStyle = classNames(
styling.tab,
selectedTab === item.label && styling.active,
!item.enabled && styling.disabled
)

return (
<li
tabIndex={0}
className={tabStyle}
onClick={() => {
if (item.enabled) {
setSelectedTab(activeTab || item.label)
if (onChangeCallback) {
onChangeCallback(activeTab || item.label)
}
}
}}
onKeyPress={e => handleKeyPress(e, item.label)}
{...rest}>
{item.label}
</li>
)
}
const handleTabChange = useCallback(
item => {
if (item.enabled) {
/* When `activeTab` prop is provided, it is a controlled component; let the parent
component handle the change. Otherwise, update the active tab internally */
if (!activeTab) {
setSelectedTab(item?.label)
}

if (onChangeCallback) {
onChangeCallback(item?.label)
}
}
},
[activeTab, onChangeCallback]
)

return (
<div className={containerStyle}>
<ul className={styling['tab-list']}>
{options.map((item, index) => (
<Tab key={index} item={item} />
<Tab
styling={styling}
isActive={selectedTab === item.label}
key={index}
item={item}
onTabChange={() => handleTabChange(item)}
Copy link
Contributor

Choose a reason for hiding this comment

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

to avoid having this be an inline anonymous function, which can effect re-renders, I think you can just make this onTabChange={handleTabChange}, because when you call it now from within the Tab component, you send the item as an argument there, which the function definition is equipped to handle

{...rest}
/>
))}
</ul>
</div>
Expand All @@ -70,7 +95,7 @@ export const Tabs = ({

Tabs.propTypes = {
/**
* Optional property that makes this a controlled component
* Optional tab option label property that makes this a controlled component
*/
activeTab: PropTypes.string,

Expand All @@ -80,7 +105,7 @@ Tabs.propTypes = {
className: PropTypes.string,

/**
* Optional property that sets the initial default value, but leaves the component uncontrolled
* Optional tab option label property that sets the initial default value, but leaves the component uncontrolled
*/
defaultActiveTab: PropTypes.string,

Expand Down
49 changes: 39 additions & 10 deletions src/stories/Tabs.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,26 @@
import React from 'react'
import { useState } from '@storybook/client-api'
import { useState, useEffect } from '@storybook/client-api'
import Tabs, { Tabs as TabsComponent } from '../core/components/tabs/Tabs'
import FormCheckbox from '../core/components/formCheckbox/FormCheckbox'

export default {
title: 'Robits/Tabs',
component: TabsComponent
}

const tabOptions = [
{ label: 'Tab 1', enabled: true },
{ label: 'Tab 2', enabled: true }
]

export const Normal = ({ themeName }) => {
const tabOptions = [
{ label: 'Tab 1', enabled: true },
{ label: 'Tab 2', enabled: true }
]
const [activeTab, setActiveTab] = useState(tabOptions[0].label)
const [isControlled, setIsControlled] = useState(false)

// Reinitialize tab state
useEffect(() => {
setActiveTab(tabOptions[0].label)
}, [isControlled])

const handleTabChange = tab => {
if (tab) {
Expand All @@ -22,12 +30,33 @@ export const Normal = ({ themeName }) => {

return (
<>
<Tabs
<FormCheckbox
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 a good idea, but should be governed as a story "Knob", rather than an on page element. See the componentKnobs of other stories like FormInput

themeName={themeName}
defaultActiveTab={activeTab}
options={tabOptions}
onChangeCallback={handleTabChange}
/>
id='is-controlled-checkbox'
toggle
onChange={() => {
setIsControlled(!isControlled)
}}
checked={isControlled}>
Is Controlled
</FormCheckbox>
{isControlled ? (
<Tabs
key='controlled-tabs'
themeName={themeName}
activeTab={activeTab}
options={tabOptions}
onChangeCallback={handleTabChange}
/>
) : (
<Tabs
key='uncontrolled-tabs'
themeName={themeName}
defaultActiveTab={activeTab}
options={tabOptions}
onChangeCallback={handleTabChange}
/>
)}
{activeTab === tabOptions[0].label ? (
<p>{tabOptions[0].label} Content</p>
) : (
Expand Down