Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/swift-kiwis-sparkle.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

TreeView: Add containIntrinsicSize prop and typeahead performance improvement
5 changes: 5 additions & 0 deletions docs/content/TreeView.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,11 @@ See [Storybook](https://primer.style/react/storybook?path=/story/components-tree
type="boolean"
description="The expanded state of the item when it is initially rendered. Use when you do not need to control the state."
/>
<PropsTableRow
name="containIntrinsicSize"
type="string"
description="The size of this item's contents. Passing this will set 'content-visiblity: auto' on the content container, delaying rendering until the item is in the viewport."
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a downside to always setting content-visibility: auto? Since the consumer doesn't control the height of TreeView.Item, it feels strange to ask the consumer to pass a height value here.

Copy link
Contributor Author

@jdrush89 jdrush89 Dec 5, 2022

Choose a reason for hiding this comment

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

The consumer does control the height by passing in whatever contents they want. Only the min-height is set. If content-visibility is always set and they've made the height larger through the contents, then the scrolling will be choppy.

/>
<PropsTableRow
name="expanded"
type="boolean"
Expand Down
31 changes: 30 additions & 1 deletion src/TreeView/TreeView.features.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -652,7 +652,7 @@ export const StressTest: Story = () => {
Directory {i}
<TreeView.SubTree>
{Array.from({length: 100}).map((_, j) => (
<TreeView.Item key={i} id={`directory-${i}/file-${j}`}>
<TreeView.Item key={j} id={`directory-${i}/file-${j}`}>
<TreeView.LeadingVisual>
<FileIcon />
</TreeView.LeadingVisual>
Expand All @@ -670,4 +670,33 @@ StressTest.parameters = {
chromatic: {disableSnapshot: true},
}

export const ContainIntrinsicSize: Story = () => {
return (
<TreeView aria-label="Files">
{Array.from({length: 10}).map((_, i) => (
<TreeView.Item key={i} id={`directory-${i}`} defaultExpanded containIntrinsicSize="2rem">
<TreeView.LeadingVisual>
<TreeView.DirectoryIcon />
</TreeView.LeadingVisual>
Directory {i}
<TreeView.SubTree>
{Array.from({length: 1000}).map((_, j) => (
<TreeView.Item key={j} id={`directory-${i}/file-${j}`} containIntrinsicSize="2rem">
<TreeView.LeadingVisual>
<FileIcon />
</TreeView.LeadingVisual>
File {j}
</TreeView.Item>
))}
</TreeView.SubTree>
</TreeView.Item>
))}
</TreeView>
)
}

ContainIntrinsicSize.parameters = {
chromatic: {disableSnapshot: true},
}

export default meta
22 changes: 22 additions & 0 deletions src/TreeView/TreeView.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,28 @@ describe('Markup', () => {
await user.click(getByText(/Item 2/))
expect(treeitem).not.toHaveAttribute('aria-expanded')
})

it('should render with containIntrinsicSize', () => {
const {getByLabelText} = renderWithTheme(
<TreeView aria-label="Test tree">
<TreeView.Item id="parent" containIntrinsicSize="2rem" defaultExpanded>
Parent
<TreeView.SubTree>
<TreeView.Item containIntrinsicSize="2rem" id="child">
Child
</TreeView.Item>
</TreeView.SubTree>
</TreeView.Item>
</TreeView>,
)

// The test runner removes the contain-intrinsic-size and content-visibility
// properties, so we can only test that the elements are still rendering.
const childItem = getByLabelText(/Child/)
expect(childItem).toBeInTheDocument()
const parentItem = getByLabelText(/Parent/)
expect(parentItem).toBeInTheDocument()
})
})

describe('Keyboard interactions', () => {
Expand Down
17 changes: 15 additions & 2 deletions src/TreeView/TreeView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,8 @@ const UlBox = styled.ul<SxProp>`
.PRIVATE_TreeView-item {
outline: none;

&:focus-visible > div {
&:focus-visible > div,
&.focus-visible > div {
box-shadow: inset 0 0 0 2px ${get(`colors.accent.fg`)};
@media (forced-colors: active) {
outline: 2px solid HighlightText;
Expand Down Expand Up @@ -293,6 +294,7 @@ Root.displayName = 'TreeView'
export type TreeViewItemProps = {
id: string
children: React.ReactNode
containIntrinsicSize?: string
current?: boolean
defaultExpanded?: boolean
expanded?: boolean
Expand All @@ -304,7 +306,16 @@ const {Slots, Slot} = createSlots(['LeadingVisual', 'TrailingVisual'])

const Item = React.forwardRef<HTMLElement, TreeViewItemProps>(
(
{id: itemId, current: isCurrentItem = false, defaultExpanded, expanded, onExpandedChange, onSelect, children},
{
id: itemId,
containIntrinsicSize,
current: isCurrentItem = false,
defaultExpanded,
expanded,
onExpandedChange,
onSelect,
children,
},
ref,
) => {
const {expandedStateCache} = React.useContext(RootContext)
Expand Down Expand Up @@ -408,6 +419,8 @@ const Item = React.forwardRef<HTMLElement, TreeViewItemProps>(
style={{
// @ts-ignore CSS custom property
'--level': level,
contentVisibility: containIntrinsicSize ? 'auto' : undefined,
containIntrinsicSize,
}}
onClick={event => {
if (onSelect) {
Expand Down
84 changes: 43 additions & 41 deletions src/TreeView/useTypeahead.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ type TypeaheadOptions = {
}

export function useTypeahead({containerRef, onFocusChange}: TypeaheadOptions) {
const [searchValue, setSearchValue] = React.useState('')
const searchValue = React.useRef('')
const timeoutRef = React.useRef(0)
const onFocusChangeRef = React.useRef(onFocusChange)
const {safeSetTimeout, safeClearTimeout} = useSafeTimeout()
Expand All @@ -18,6 +18,44 @@ export function useTypeahead({containerRef, onFocusChange}: TypeaheadOptions) {
onFocusChangeRef.current = onFocusChange
}, [onFocusChange])

// Focus the closest element that matches the search value
const focusSearchValue = React.useCallback(
(searchValue: string) => {
// Don't change focus if the search value is empty
if (!searchValue) return

if (!containerRef.current) return
const container = containerRef.current

// Get focusable elements
const elements = Array.from(container.querySelectorAll('[role="treeitem"]'))

// Get the index of active element
const activeIndex = elements.findIndex(element => element === document.activeElement)

// Wrap the array elements such that the active descendant is at the beginning
let sortedElements = wrapArray(elements, activeIndex)

// Remove the active descendant from the beginning of the array
// when the user initiates a new search
if (searchValue.length === 1) {
sortedElements = sortedElements.slice(1)
}

// Find the first element that matches the search value
const nextElement = sortedElements.find(element => {
const name = getAccessibleName(element).toLowerCase()
return name.startsWith(searchValue.toLowerCase())
})

// If a match is found, focus it
if (nextElement) {
onFocusChangeRef.current(nextElement)
}
},
[containerRef],
)

// Update the search value when the user types
React.useEffect(() => {
if (!containerRef.current) return
Expand All @@ -31,11 +69,12 @@ export function useTypeahead({containerRef, onFocusChange}: TypeaheadOptions) {
if (event.ctrlKey || event.altKey || event.metaKey) return

// Update the existing search value with the new key press
setSearchValue(value => value + event.key)
searchValue.current += event.key
focusSearchValue(searchValue.current)

// Reset the timeout
safeClearTimeout(timeoutRef.current)
timeoutRef.current = safeSetTimeout(() => setSearchValue(''), 300)
timeoutRef.current = safeSetTimeout(() => (searchValue.current = ''), 300)

// Prevent default behavior
event.preventDefault()
Expand All @@ -44,44 +83,7 @@ export function useTypeahead({containerRef, onFocusChange}: TypeaheadOptions) {

container.addEventListener('keydown', onKeyDown)
return () => container.removeEventListener('keydown', onKeyDown)
}, [containerRef, safeClearTimeout, safeSetTimeout])

// Update focus when the search value changes
React.useEffect(() => {
// Don't change focus if the search value is empty
if (!searchValue) return

if (!containerRef.current) return
const container = containerRef.current

// Get focusable elements
const elements = Array.from(container.querySelectorAll('[role="treeitem"]'))
// Filter out collapsed items
.filter(element => !element.parentElement?.closest('[role=treeitem][aria-expanded=false]'))

// Get the index of active element
const activeIndex = elements.findIndex(element => element === document.activeElement)

// Wrap the array elements such that the active descendant is at the beginning
let sortedElements = wrapArray(elements, activeIndex)

// Remove the active descendant from the beginning of the array
// when the user initiates a new search
if (searchValue.length === 1) {
sortedElements = sortedElements.slice(1)
}

// Find the first element that matches the search value
const nextElement = sortedElements.find(element => {
const name = getAccessibleName(element).toLowerCase()
return name.startsWith(searchValue.toLowerCase())
})

// If a match is found, focus it
if (nextElement) {
onFocusChangeRef.current(nextElement)
}
}, [searchValue, containerRef])
}, [containerRef, focusSearchValue, safeClearTimeout, safeSetTimeout])
}

/**
Expand Down