From 50ed644180733ab7baa8ee79dc019a9b6f61bf99 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Thu, 3 Nov 2022 16:08:07 -0700 Subject: [PATCH] TreeView: Performance improvements (#2523) * Prevent focus event from bubbling to parent items * Update scroll story * Create violet-plants-sip.md * Ignore lint error * Update stress test * Lift styles to root * Remove unused imports * Remove unused sx prop * Clean up styles * Create tidy-olives-know.md * Update warning and prefix classnames with PRIVATE_ * Replace VisuallyHidden styled-component * Use VisuallyHidden component for root level live region * Update warning * Style directory icon without styled-components * Remove sx prop from item * Disable chromatic on TreeView stress test --- .changeset/tidy-olives-know.md | 5 + src/TreeView/TreeView.stories.tsx | 3 +- src/TreeView/TreeView.tsx | 408 +++++++++++++++++------------- 3 files changed, 238 insertions(+), 178 deletions(-) create mode 100644 .changeset/tidy-olives-know.md diff --git a/.changeset/tidy-olives-know.md b/.changeset/tidy-olives-know.md new file mode 100644 index 00000000000..0ac4ade72c3 --- /dev/null +++ b/.changeset/tidy-olives-know.md @@ -0,0 +1,5 @@ +--- +"@primer/react": patch +--- + +TreeView: Performance improvements diff --git a/src/TreeView/TreeView.stories.tsx b/src/TreeView/TreeView.stories.tsx index fc317019893..f19899fd144 100644 --- a/src/TreeView/TreeView.stories.tsx +++ b/src/TreeView/TreeView.stories.tsx @@ -606,7 +606,7 @@ export const StressTest: Story = () => { Directory {index} - {Array.from({length: 100}).map((_, index) => ( + {Array.from({length: 1000}).map((_, index) => ( @@ -623,7 +623,6 @@ export const StressTest: Story = () => { } StressTest.parameters = { - // disables Chromatic's snapshotting on a story level chromatic: {disableSnapshot: true} } diff --git a/src/TreeView/TreeView.tsx b/src/TreeView/TreeView.tsx index 04d4e6a6601..ae9118c8e64 100644 --- a/src/TreeView/TreeView.tsx +++ b/src/TreeView/TreeView.tsx @@ -5,18 +5,16 @@ import { FileDirectoryOpenFillIcon } from '@primer/octicons-react' import {useSSRSafeId} from '@react-aria/ssr' +import classnames from 'classnames' import React from 'react' import styled, {keyframes} from 'styled-components' -import Box from '../Box' -import {ConfirmationDialog} from '../Dialog/ConfirmationDialog' import {get} from '../constants' +import {ConfirmationDialog} from '../Dialog/ConfirmationDialog' import {useControllableState} from '../hooks/useControllableState' import useSafeTimeout from '../hooks/useSafeTimeout' import Spinner from '../Spinner' -import StyledOcticon from '../StyledOcticon' -import sx, {SxProp, merge} from '../sx' +import sx, {SxProp} from '../sx' import Text from '../Text' -import {Theme} from '../ThemeProvider' import createSlots from '../utils/create-slots' import VisuallyHidden from '../_VisuallyHidden' import {getAccessibleName} from './shared' @@ -61,7 +59,184 @@ export type TreeViewProps = { children: React.ReactNode } -const UlBox = styled.ul(sx) +const UlBox = styled.ul` + list-style: none; + padding: 0; + margin: 0; + + /* + * WARNING: This is a performance optimization. + * + * We define styles for the tree items at the root level of the tree + * to avoid recomputing the styles for each item when the tree updates. + * We're sacraficing maintainability for performance because TreeView + * needs to be performant enough to handle large trees (thousands of items). + * + * This is intended to be a temporary solution until we can improve the + * performance of our styling patterns. + * + * Do NOT copy this pattern without understanding the tradeoffs. + * Do NOT reference PRIVATE_* classnames outside of this file. + */ + .PRIVATE_TreeView-item { + outline: none; + + &:focus-visible > div { + box-shadow: inset 0 0 0 2px ${get(`colors.accent.fg`)}; + @media (forced-colors: active) { + outline: 2px solid HighlightText; + outline-offset: -2; + } + } + } + + .PRIVATE_TreeView-item-container { + --level: 1; /* default level */ + --toggle-width: 1rem; /* 16px */ + position: relative; + display: grid; + grid-template-columns: calc(calc(var(--level) - 1) * (var(--toggle-width) / 2)) var(--toggle-width) 1fr; + grid-template-areas: 'spacer toggle content'; + width: 100%; + min-height: 2rem; /* 32px */ + font-size: ${get('fontSizes.1')}; + color: ${get('colors.fg.default')}; + border-radius: ${get('radii.2')}; + cursor: pointer; + + &:hover { + background-color: ${get('colors.actionListItem.default.hoverBg')}; + + @media (forced-colors: active) { + outline: 2px solid transparent; + outline-offset: -2px; + } + } + + @media (pointer: coarse) { + --toggle-width: 1.5rem; /* 24px */ + min-height: 2.75rem; /* 44px */ + } + + &:has(.PRIVATE_TreeView-item-skeleton):hover { + background-color: transparent; + cursor: default; + + @media (forced-colors: active) { + outline: none; + } + } + } + + .PRIVATE_TreeView-item[aria-current='true'] > .PRIVATE_TreeView-item-container { + background-color: ${get('colors.actionListItem.default.selectedBg')}; + + /* Current item indicator */ + &::after { + content: ''; + position: absolute; + top: calc(50% - 0.75rem); /* 50% - 12px */ + left: -${get('space.2')}; + width: 0.25rem; /* 4px */ + height: 1.5rem; /* 24px */ + background-color: ${get('colors.accent.fg')}; + border-radius: ${get('radii.2')}; + + @media (forced-colors: active) { + background-color: HighlightText; + } + } + } + + .PRIVATE_TreeView-item-toggle { + grid-area: toggle; + display: flex; + align-items: center; + justify-content: center; + height: 100%; + color: ${get('colors.fg.muted')}; + } + + .PRIVATE_TreeView-item-toggle--hover:hover { + background-color: ${get('colors.treeViewItem.chevron.hoverBg')}; + } + + .PRIVATE_TreeView-item-toggle--end { + border-top-left-radius: ${get('radii.2')}; + border-bottom-left-radius: ${get('radii.2')}; + } + + .PRIVATE_TreeView-item-content { + grid-area: content; + display: flex; + align-items: center; + height: 100%; + padding: 0 ${get('space.2')}; + gap: ${get('space.2')}; + } + + .PRIVATE_TreeView-item-content-text { + /* Truncate text label */ + flex: 1 1 auto; + width: 0; + overflow: hidden; + white-space: nowrap; + text-overflow: ellipsis; + } + + .PRIVATE_TreeView-item-visual { + display: flex; + color: ${get('colors.fg.muted')}; + } + + .PRIVATE_TreeView-item-level-line { + width: 100%; + height: 100%; + border-right: 1px solid; + + /* + * On devices without hover, the nesting indicator lines + * appear at all times. + */ + border-color: ${get('colors.border.subtle')}; + } + + /* + * On devices with :hover support, the nesting indicator lines + * fade in when the user mouses over the entire component, + * or when there's focus inside the component. This makes + * sure the component remains simple when not in use. + */ + @media (hover: hover) { + .PRIVATE_TreeView-item-level-line { + border-color: transparent; + } + + &:hover .PRIVATE_TreeView-item-level-line, + &:focus-within .PRIVATE_TreeView-item-level-line { + border-color: ${get('colors.border.subtle')}; + } + } + + .PRIVATE_TreeView-directory-icon { + display: grid; + color: ${get('colors.treeViewItem.directory.fill')}; + } + + .PRIVATE_VisuallyHidden { + position: absolute; + width: 1px; + height: 1px; + padding: 0; + margin: -1px; + overflow: hidden; + clip: rect(0, 0, 0, 0); + white-space: nowrap; + border-width: 0; + } + + ${sx} +` const Root: React.FC = ({'aria-label': ariaLabel, 'aria-labelledby': ariaLabelledby, children}) => { const containerRef = React.useRef(null) @@ -88,17 +263,7 @@ const Root: React.FC = ({'aria-label': ariaLabel, 'aria-labelledb {ariaLiveMessage} - + {children} @@ -118,15 +283,12 @@ export type TreeViewItemProps = { expanded?: boolean onExpandedChange?: (expanded: boolean) => void onSelect?: (event: React.MouseEvent | React.KeyboardEvent) => void -} & SxProp +} const {Slots, Slot} = createSlots(['LeadingVisual', 'TrailingVisual']) const Item = React.forwardRef( - ( - {current: isCurrentItem = false, defaultExpanded = false, expanded, onExpandedChange, onSelect, children, sx = {}}, - ref - ) => { + ({current: isCurrentItem = false, defaultExpanded = false, expanded, onExpandedChange, onSelect, children}, ref) => { const itemId = useSSRSafeId() const labelId = useSSRSafeId() const leadingVisualId = useSSRSafeId() @@ -203,9 +365,9 @@ const Item = React.forwardRef( }} > {/* @ts-ignore Box doesn't have type support for `ref` used in combination with `as` */} - } tabIndex={0} id={itemId} role="treeitem" @@ -222,18 +384,14 @@ const Item = React.forwardRef( // Prevent focus event from bubbling up to parent items event.stopPropagation() }} - sx={{ - outline: 'none', - '&:focus-visible > div': { - boxShadow: (theme: Theme) => `inset 0 0 0 2px ${theme.colors.accent.fg}`, - '@media (forced-colors: active)': { - outline: '2px solid HighlightText', - outlineOffset: -2 - } - } - }} > - { if (onSelect) { onSelect(event) @@ -241,112 +399,41 @@ const Item = React.forwardRef( toggle(event) } }} - sx={merge.all([ - { - '--toggle-width': '1rem', // 16px - position: 'relative', - display: 'grid', - gridTemplateColumns: `calc(${level - 1} * (var(--toggle-width) / 2)) var(--toggle-width) 1fr`, - gridTemplateAreas: `"spacer toggle content"`, - width: '100%', - minHeight: '2rem', // 32px - fontSize: 1, - color: 'fg.default', - borderRadius: 2, - cursor: 'pointer', - '&:hover': { - backgroundColor: 'actionListItem.default.hoverBg', - '@media (forced-colors: active)': { - outline: '2px solid transparent', - outlineOffset: -2 - } - }, - '@media (pointer: coarse)': { - '--toggle-width': '1.5rem', // 24px - minHeight: '2.75rem' // 44px - }, - '[role=treeitem][aria-current=true] > &:is(div)': { - bg: 'actionListItem.default.selectedBg', - '&::after': { - position: 'absolute', - top: 'calc(50% - 12px)', - left: -2, - width: '4px', - height: '24px', - content: '""', - bg: 'accent.fg', - borderRadius: 2, - '@media (forced-colors: active)': { - backgroundColor: 'HighlightText' - } - } - } - }, - sx as SxProp - ])} > - +
- +
{hasSubTree ? ( - { if (onSelect) { toggle(event) } }} - sx={{ - gridArea: 'toggle', - display: 'flex', - alignItems: 'center', - justifyContent: 'center', - height: '100%', - color: 'fg.muted', - borderTopLeftRadius: level === 1 ? 2 : 0, - borderBottomLeftRadius: level === 1 ? 2 : 0, - '&:hover': { - backgroundColor: onSelect ? 'treeViewItem.chevron.hoverBg' : null - } - }} > {isExpanded ? : } - + ) : null} - +
{slots => ( <> {slots.LeadingVisual} - - {childrenWithoutSubTree} - + {childrenWithoutSubTree} {slots.TrailingVisual} )} - - +
+ {subTree} - + ) } @@ -355,33 +442,11 @@ const Item = React.forwardRef( /** Lines to indicate the depth of an item in a TreeView */ const LevelIndicatorLines: React.FC<{level: number}> = ({level}) => { return ( - +
{Array.from({length: level - 1}).map((_, index) => ( - +
))} - +
) } @@ -503,11 +568,9 @@ const SubTree: React.FC = ({count, state, children}) => { } return ( - + ) } @@ -527,7 +590,7 @@ const shimmer = keyframes` to { mask-position: 0%; } ` -const SkeletonItem = styled.span` +const SkeletonItem = styled.span.attrs({className: 'PRIVATE_TreeView-item-skeleton'})` display: flex; align-items: center; column-gap: 0.5rem; @@ -601,22 +664,11 @@ const LoadingItem = React.forwardRef((props, ref) if (count) { return ( - + {Array.from({length: count}).map((_, i) => { return })} - Loading {count} items +
Loading {count} items
 ) } @@ -664,12 +716,12 @@ const LeadingVisual: React.FC = props => { const children = typeof props.children === 'function' ? props.children({isExpanded}) : props.children return ( - +
{props.label} - - +
+
{children} - +
) } @@ -681,12 +733,12 @@ const TrailingVisual: React.FC = props => { const children = typeof props.children === 'function' ? props.children({isExpanded}) : props.children return ( - +
{props.label} - - +
+
{children} - +
) } @@ -698,8 +750,12 @@ TrailingVisual.displayName = 'TreeView.TrailingVisual' const DirectoryIcon = () => { const {isExpanded} = React.useContext(ItemContext) - const icon = isExpanded ? FileDirectoryOpenFillIcon : FileDirectoryFillIcon - return + const Icon = isExpanded ? FileDirectoryOpenFillIcon : FileDirectoryFillIcon + return ( +
+ +
+ ) } // ----------------------------------------------------------------------------