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

Jump to bottom

TreeView: Improve performance when rendering lots of items #2460

Merged
merged 7 commits into from
Oct 20, 2022
Merged

TreeView: Improve performance when rendering lots of items #2460

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c30630d
Avoid unecessary style recalculation
colebemis Oct 20, 2022
02248cd
Add stress test story
colebemis Oct 20, 2022
dc9b785
Only render subtree if it's expanded
colebemis Oct 20, 2022
c0bdea1
Don't expand current item by default
colebemis Oct 20, 2022
299b42b
Update controlled story
colebemis Oct 20, 2022
850625f
Create popular-taxis-yawn.md
colebemis Oct 20, 2022
3d4566b
Merge remote-tracking branch 'origin/main' into treeview-lots-of-items
colebemis Oct 20, 2022
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
5 changes: 5 additions & 0 deletions .changeset/popular-taxis-yawn.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

TreeView: Improve performance when rendering lots of items
139 changes: 46 additions & 93 deletions src/TreeView/TreeView.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import {DiffAddedIcon, DiffModifiedIcon, DiffRemovedIcon, DiffRenamedIcon, FileIcon} from '@primer/octicons-react'
import {Meta, Story} from '@storybook/react'
import React from 'react'
import {ActionList} from '../ActionList'
import {ActionMenu} from '../ActionMenu'
import Box from '../Box'
import {Button} from '../Button'
import {ConfirmationDialog} from '../Dialog/ConfirmationDialog'
Expand All @@ -21,7 +19,7 @@ export const FileTreeWithDirectoryLinks: Story = () => (
<Box sx={{p: 3, maxWidth: 400}}>
<nav aria-label="File navigation">
<TreeView aria-label="File navigation">
<TreeView.LinkItem href="#src">
<TreeView.LinkItem href="#src" defaultExpanded>
<TreeView.LeadingVisual>
<TreeView.DirectoryIcon />
</TreeView.LeadingVisual>
Expand Down Expand Up @@ -242,107 +240,35 @@ const CurrentPathContext = React.createContext<{
setCurrentPath: () => {}
})

export const Controlled: Story = () => {
const [currentPath, setCurrentPath] = React.useState<string[]>(['src', 'Avatar.tsx'])
const [tree, setTree] = React.useState<TreeItem[]>([
{
data: {
name: 'src',
expanded: false
},
children: [
{
data: {
name: 'Avatar.tsx',
expanded: false
},
children: []
},
{
data: {
name: 'Button',
expanded: false
},
children: [
{
data: {
name: 'Button.tsx',
expanded: false
},
children: []
},
{
data: {
name: 'Button.test.tsx',
expanded: false
},
children: []
}
]
}
]
},
{
data: {
name: 'public',
expanded: false
},
children: [
{
data: {
name: 'index.html',
expanded: false
},
children: []
},
{
data: {
name: 'favicon.ico',
expanded: false
},
children: []
}
]
const TREE: TreeItem[] = Array.from({length: 5}).map((_, i) => ({
data: {
name: `Item ${i}`,
expanded: false
},
children: Array.from({length: 5}).map((_, j) => ({
data: {
name: `Item ${i}.${j}`,
expanded: false
},
{
children: Array.from({length: 5}).map((_, k) => ({
data: {
name: 'package.json',
name: `Item ${i}.${j}.${k}`,
expanded: false
},
children: []
}
])
}))
}))
}))

export const Controlled: Story = () => {
const [currentPath, setCurrentPath] = React.useState<string[]>(['src', 'Avatar.tsx'])
const [tree, setTree] = React.useState<TreeItem[]>(TREE)

return (
<Box sx={{p: 3, display: 'grid', gap: 3}}>
<Box sx={{display: 'flex', gap: 2}}>
<Button onClick={() => setTree(collapseAll)}>Collapse all</Button>
<Button onClick={() => setTree(expandAll)}>Expand all</Button>
<ActionMenu>
<ActionMenu.Button>Jump to</ActionMenu.Button>

<ActionMenu.Overlay>
<ActionList>
<ActionList.Item onSelect={() => setCurrentPath(['src'])}>src</ActionList.Item>
<ActionList.Item onSelect={() => setCurrentPath(['src', 'Avatar.tsx'])}>src/Avatar.tsx</ActionList.Item>
<ActionList.Item onSelect={() => setCurrentPath(['src', 'Button'])}>src/Button</ActionList.Item>
<ActionList.Item onSelect={() => setCurrentPath(['src', 'Button', 'Button.tsx'])}>
src/Button/Button.tsx
</ActionList.Item>
<ActionList.Item onSelect={() => setCurrentPath(['src', 'Button', 'Button.test.tsx'])}>
src/Button/Button.test.tsx
</ActionList.Item>
<ActionList.Item onSelect={() => setCurrentPath(['public'])}>public</ActionList.Item>
<ActionList.Item onSelect={() => setCurrentPath(['public', 'index.html'])}>
public/index.html
</ActionList.Item>
<ActionList.Item onSelect={() => setCurrentPath(['public', 'favicon.ico'])}>
public/favicon.ico
</ActionList.Item>
<ActionList.Item onSelect={() => setCurrentPath(['package.json'])}>package.json</ActionList.Item>
</ActionList>
</ActionMenu.Overlay>
</ActionMenu>
</Box>
<nav aria-label="File navigation">
<CurrentPathContext.Provider value={{currentPath, setCurrentPath}}>
Expand Down Expand Up @@ -553,4 +479,31 @@ AsyncError.args = {
responseTime: 2000
}

export const StressTest: Story = () => {
return (
<Box sx={{p: 3, maxWidth: 400}}>
<TreeView aria-label="Files">
{Array.from({length: 1000}).map((_, index) => (
<TreeView.Item key={index}>
<TreeView.LeadingVisual>
<TreeView.DirectoryIcon />
</TreeView.LeadingVisual>
Directory {index}
<TreeView.SubTree>
{Array.from({length: 100}).map((_, index) => (
<TreeView.Item key={index}>
<TreeView.LeadingVisual>
<FileIcon />
</TreeView.LeadingVisual>
File {index}
</TreeView.Item>
))}
</TreeView.SubTree>
</TreeView.Item>
))}
</TreeView>
</Box>
)
}

export default meta
133 changes: 0 additions & 133 deletions src/TreeView/TreeView.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,139 +73,6 @@ describe('Markup', () => {
expect(currentItem).toHaveAttribute('aria-current', 'true')
})

it('expands the path to the current item (level 2) by default', () => {
const {getByRole} = renderWithTheme(
<TreeView aria-label="Test tree">
<TreeView.Item>
Item 1
<TreeView.SubTree>
<TreeView.Item>Item 1.1</TreeView.Item>
</TreeView.SubTree>
</TreeView.Item>
<TreeView.Item>
Item 2
<TreeView.SubTree>
<TreeView.Item>Item 2.1</TreeView.Item>
<TreeView.Item current>
Item 2.2
<TreeView.SubTree>
<TreeView.Item>Item 2.2.1</TreeView.Item>
</TreeView.SubTree>
</TreeView.Item>
</TreeView.SubTree>
</TreeView.Item>
<TreeView.Item>Item 3</TreeView.Item>
</TreeView>
)

const item1 = getByRole('treeitem', {name: 'Item 1'})
const item2 = getByRole('treeitem', {name: 'Item 2'})
const item22 = getByRole('treeitem', {name: 'Item 2.2'})
const item221 = getByRole('treeitem', {name: 'Item 2.2.1'})

// Item 1 should not be expanded because it is not the parent of the current item
expect(item1).toHaveAttribute('aria-expanded', 'false')

// Item 2 should be expanded because it is the parent of the current item
expect(item2).toHaveAttribute('aria-expanded', 'true')

// Item 2.2 should be expanded because it is the current item
expect(item22).toHaveAttribute('aria-expanded', 'true')

// Item 2.2 should have an aria-current value of true
expect(item22).toHaveAttribute('aria-current', 'true')

// Item 2.2.1 should be visible because it is a child of the current item
expect(item221).toBeVisible()
})

it('expands the path to the current item (level 3) by default', () => {
const {getByRole} = renderWithTheme(
<TreeView aria-label="Test tree">
<TreeView.Item>
Item 1
<TreeView.SubTree>
<TreeView.Item>Item 1.1</TreeView.Item>
</TreeView.SubTree>
</TreeView.Item>
<TreeView.Item>
Item 2
<TreeView.SubTree>
<TreeView.Item>Item 2.1</TreeView.Item>
<TreeView.Item>
Item 2.2
<TreeView.SubTree>
<TreeView.Item current>Item 2.2.1</TreeView.Item>
</TreeView.SubTree>
</TreeView.Item>
</TreeView.SubTree>
</TreeView.Item>
<TreeView.Item>Item 3</TreeView.Item>
</TreeView>
)

const item1 = getByRole('treeitem', {name: 'Item 1'})
const item2 = getByRole('treeitem', {name: 'Item 2'})
const item22 = getByRole('treeitem', {name: 'Item 2.2'})
const item221 = getByRole('treeitem', {name: 'Item 2.2.1'})

// Item 1 should not be expanded because it is not the parent of the current item
expect(item1).toHaveAttribute('aria-expanded', 'false')

// Item 2 should be expanded because it is the parent of the current item
expect(item2).toHaveAttribute('aria-expanded', 'true')

// Item 2.2 should be expanded because it is the current item
expect(item22).toHaveAttribute('aria-expanded', 'true')

// Item 2.2.1 should be the current item
expect(item221).toHaveAttribute('aria-current', 'true')
})

it('expands the path to the current item when the current item is changed', () => {
function TestTree() {
const [current, setCurrent] = React.useState('item1')
return (
<div>
<button onClick={() => setCurrent('item2')}>Jump to Item 2</button>
<TreeView aria-label="Test tree">
<TreeView.Item current={current === 'item1'}>Item 1</TreeView.Item>
<TreeView.Item current={current === 'item2'}>
Item 2
<TreeView.SubTree>
<TreeView.Item current={current === 'item2.1'}>Item 2.1</TreeView.Item>
</TreeView.SubTree>
</TreeView.Item>
<TreeView.Item current={current === 'item3'}>Item 3</TreeView.Item>
</TreeView>
</div>
)
}

const {getByRole, getByText} = renderWithTheme(<TestTree />)

const item1 = getByRole('treeitem', {name: 'Item 1'})
const item2 = getByRole('treeitem', {name: 'Item 2'})

// Item 1 should have an aria-current value of true
expect(item1).toHaveAttribute('aria-current', 'true')

// Item 2 should not be expanded because it is not the current item or the parent of the current item
expect(item2).toHaveAttribute('aria-expanded', 'false')

// Click the button to change the current item to Item 2
fireEvent.click(getByText('Jump to Item 2'))

// Item 1 should not have an aria-current value
expect(item1).not.toHaveAttribute('aria-current')

// Item 2 should be expanded because it is the current item
expect(item2).toHaveAttribute('aria-expanded', 'true')

// Item 2.1 should be visible because it is a child of the current item
expect(getByRole('treeitem', {name: 'Item 2.1'})).toBeVisible()
})

it('should be described by leading visuals', () => {
const {getByLabelText} = renderWithTheme(
<TreeView aria-label="Test tree">
Expand Down
Loading