From c30630dd82a2fae475de74745095deb942eae01b Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 20 Oct 2022 10:49:51 -0700 Subject: [PATCH 01/11] Avoid unecessary style recalculation --- src/TreeView/TreeView.tsx | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index d2afa327788..505d9475146 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -198,8 +198,10 @@ const Item = React.forwardRef( trailingVisualId }} > -
  • } + ( aria-level={level} aria-expanded={hasSubTree ? isExpanded : undefined} aria-current={isCurrentItem ? 'true' : undefined} - style={{outline: 'none'}} onKeyDown={handleKeyDown} + sx={{ + outline: 'none', + '&:focus-visible > div': { + boxShadow: (theme: Theme) => `inset 0 0 0 2px ${theme.colors.accent.emphasis}`, + '@media (forced-colors: active)': { + outline: '2px solid SelectedItem', + outlineOffset: -2 + } + } + }} > { @@ -242,17 +253,6 @@ const Item = React.forwardRef( '--toggle-width': '1.5rem', // 24px height: '2.75rem' // 44px }, - // WARNING: styled-components v5.2 introduced a bug that changed - // how it expands `&` in CSS selectors. The following selectors - // are unnecessarily specific to work around that styled-components bug. - // Reference issue: https://github.com/styled-components/styled-components/issues/3265 - [`#${itemId}:focus-visible > &:is(div)`]: { - boxShadow: (theme: Theme) => `inset 0 0 0 2px ${theme.colors.accent.emphasis}`, - '@media (forced-colors: active)': { - outline: '2px solid SelectedItem', - outlineOffset: -2 - } - }, '[role=treeitem][aria-current=true] > &:is(div)': { bg: 'actionListItem.default.selectedBg', '&::after': { From 02248cdb7923c0c18c0848f1d426bc51e25f67e0 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 20 Oct 2022 10:54:09 -0700 Subject: [PATCH 02/11] Add stress test story --- src/TreeView/TreeView.stories.tsx | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/TreeView/TreeView.stories.tsx b/src/TreeView/TreeView.stories.tsx index a153b1dba71..c9619a61c20 100644 --- a/src/TreeView/TreeView.stories.tsx +++ b/src/TreeView/TreeView.stories.tsx @@ -553,4 +553,31 @@ AsyncError.args = { responseTime: 2000 } +export const StressTest: Story = () => { + return ( + + + {Array.from({length: 1000}).map((_, index) => ( + + + + + Directory {index} + + {Array.from({length: 100}).map((_, index) => ( + + + + + File {index} + + ))} + + + ))} + + + ) +} + export default meta From dc9b785c9e0f9461a390522c5f06b6cc166cecad Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 20 Oct 2022 11:02:59 -0700 Subject: [PATCH 03/11] Only render subtree if it's expanded --- src/TreeView/TreeView.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index 505d9475146..8c1dacf1661 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -328,8 +328,8 @@ const Item = React.forwardRef( - {subTree} -
    • + {isExpanded ? subTree : null} + ) } From c0bdea13ed1be9b162ae943b6c072adfa749bf3c Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 20 Oct 2022 11:22:24 -0700 Subject: [PATCH 04/11] Don't expand current item by default --- src/TreeView/TreeView.test.tsx | 133 --------------------------------- src/TreeView/TreeView.tsx | 23 ++---- 2 files changed, 7 insertions(+), 149 deletions(-) diff --git a/src/TreeView/TreeView.test.tsx b/src/TreeView/TreeView.test.tsx index 0fcbfd2640f..ec950a0f660 100644 --- a/src/TreeView/TreeView.test.tsx +++ b/src/TreeView/TreeView.test.tsx @@ -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( - - - Item 1 - - Item 1.1 - - - - Item 2 - - Item 2.1 - - Item 2.2 - - Item 2.2.1 - - - - - Item 3 - - ) - - 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( - - - Item 1 - - Item 1.1 - - - - Item 2 - - Item 2.1 - - Item 2.2 - - Item 2.2.1 - - - - - Item 3 - - ) - - 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 ( -
    - - - Item 1 - - Item 2 - - Item 2.1 - - - Item 3 - -
    - ) - } - - const {getByRole, getByText} = renderWithTheme() - - 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( diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index 8c1dacf1661..093fde6e85a 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -34,14 +34,12 @@ const ItemContext = React.createContext<{ itemId: string level: number isExpanded: boolean - expandParents: () => void leadingVisualId: string trailingVisualId: string }>({ itemId: '', level: 1, isExpanded: false, - expandParents: () => {}, leadingVisualId: '', trailingVisualId: '' }) @@ -128,7 +126,7 @@ const Item = React.forwardRef( value: expanded, onChange: onExpandedChange }) - const {level, expandParents} = React.useContext(ItemContext) + const {level} = React.useContext(ItemContext) const {hasSubTree, subTree, childrenWithoutSubTree} = useSubTree(children) // Expand or collapse the subtree @@ -142,24 +140,18 @@ const Item = React.forwardRef( [isExpanded] ) - // Expand all parents of this item including itself - const expandParentsAndSelf = React.useCallback( + // If this item is the current item, expand it + React.useLayoutEffect( () => { - expandParents() - setIsExpanded(true) + if (isCurrentItem) { + setIsExpanded(true) + } }, // setIsExpanded is stable // eslint-disable-next-line react-hooks/exhaustive-deps - [expandParents] + [isCurrentItem] ) - // If this item is the current item, expand it and all its parents - React.useLayoutEffect(() => { - if (isCurrentItem) { - expandParentsAndSelf() - } - }, [isCurrentItem, expandParentsAndSelf]) - const handleKeyDown = React.useCallback( (event: React.KeyboardEvent) => { switch (event.key) { @@ -193,7 +185,6 @@ const Item = React.forwardRef( itemId, level: level + 1, isExpanded, - expandParents: expandParentsAndSelf, leadingVisualId, trailingVisualId }} From 299b42bcdf579b28fc61b409c198f94545215752 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 20 Oct 2022 11:24:42 -0700 Subject: [PATCH 05/11] Update controlled story --- src/TreeView/TreeView.stories.tsx | 112 +++++------------------------- 1 file changed, 19 insertions(+), 93 deletions(-) diff --git a/src/TreeView/TreeView.stories.tsx b/src/TreeView/TreeView.stories.tsx index c9619a61c20..d27990d896c 100644 --- a/src/TreeView/TreeView.stories.tsx +++ b/src/TreeView/TreeView.stories.tsx @@ -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' @@ -21,7 +19,7 @@ export const FileTreeWithDirectoryLinks: Story = () => (