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

Add TreeView.LeadingAction sub-component #4546

Merged
merged 20 commits into from
May 15, 2024
Merged

Add TreeView.LeadingAction sub-component #4546

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
a5ccd2d
add grid area for drag handle when data-drag-and-drop is true
ayy-bc Apr 30, 2024
66c876d
add test to verify dnd attribute
ayy-bc Apr 30, 2024
c982c1d
css updates
ayy-bc Apr 30, 2024
61b2e50
add docs
ayy-bc Apr 30, 2024
bba15da
test(vrt): update snapshots
ayy-bc Apr 30, 2024
1794aa5
Sid/treeview leading action (#4569)
siddharthkp May 8, 2024
5d54533
merge snapshots from main
siddharthkp May 8, 2024
069b72a
merge package-lock from main
siddharthkp May 8, 2024
dd39abb
Merge branch 'main' into ayy-bc/addTreeViewDragHandle
siddharthkp May 8, 2024
f5e3dde
add visual tests
siddharthkp May 8, 2024
a8a3069
use IconButton for leadingAction
siddharthkp May 8, 2024
1f63a8b
add example of drag handle on hover
siddharthkp May 8, 2024
f2442b7
Create tame-nails-live.md
siddharthkp May 8, 2024
920ea88
test(vrt): update snapshots
siddharthkp May 8, 2024
770e321
change LeadingActio type to React.FC<TreeViewVisualProps> and accept …
ayy-bc May 9, 2024
f9b880d
add leadingActionId and aria-hidden to LeadingAction subcomponent
ayy-bc May 9, 2024
3771156
remove `leadingActionId` to describe the tree view item
ayy-bc May 14, 2024
c8b1086
remove unused leadingActionId
siddharthkp May 15, 2024
b438d27
remove docs from changeset
siddharthkp May 15, 2024
29348d6
Merge branch 'main' into ayy-bc/addTreeViewDragHandle
siddharthkp May 15, 2024
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
4 changes: 2 additions & 2 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.

92 changes: 92 additions & 0 deletions packages/react/src/TreeView/TreeView.features.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
DiffRemovedIcon,
DiffRenamedIcon,
FileIcon,
GrabberIcon,
KebabHorizontalIcon,
} from '@primer/octicons-react'
import type {Meta, Story} from '@storybook/react'
Expand Down Expand Up @@ -695,6 +696,97 @@ export const NestedTrees: Story = () => {
)
}

export const WithDragHandle: Story = () => {
const dragHandle = (
<IconButton
sx={{p: 1, cursor: 'grab', mr: 1}}
aria-label={`Move`}
variant="invisible"
// When dragging, the button is no longer a button, but has the role application to allow
// keyboard movements to not be registered by the screen reader
icon={GrabberIcon}
size="large"
/>
)

const [isLoading, setIsLoading] = React.useState(false)
const [asyncItems, setAsyncItems] = React.useState<string[]>([])

let state: SubTreeState = 'initial'

if (isLoading) {
state = 'loading'
} else if (asyncItems.length > 0) {
state = 'done'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to include the loading functionality for this story. We can just focus on the drag-and-drop behavior.

Copy link
Member

Choose a reason for hiding this comment

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

+1


return (
<nav aria-label="Files">
<TreeView aria-label="Files" dragAndDrop>
<TreeView.Item id="file-1" dragHandle={dragHandle}>
<TreeView.LeadingVisual>
<FileIcon />
</TreeView.LeadingVisual>
Some file
</TreeView.Item>
<TreeView.Item
id="async-directory"
dragHandle={dragHandle}
onExpandedChange={async isExpanded => {
if (asyncItems.length === 0 && isExpanded) {
setIsLoading(true)

// Load items
const items = await loadItems(1000)

setIsLoading(false)
setAsyncItems(items)
}
}}
>
<TreeView.LeadingVisual>
<TreeView.DirectoryIcon />
</TreeView.LeadingVisual>
Directory with async items
<TreeView.SubTree state={state}>
{asyncItems.map(item => (
<TreeView.Item id={`item-${item}`} key={item}>
<TreeView.LeadingVisual>
<FileIcon />
</TreeView.LeadingVisual>
{item}
</TreeView.Item>
))}
<TreeView.Item id="nested-directory">
Nested Sub-tree
<TreeView.SubTree state="done">
<TreeView.Item id="nested-directory/file-1">
<TreeView.LeadingVisual>
<FileIcon />
</TreeView.LeadingVisual>
Some file
</TreeView.Item>
<TreeView.Item id="nested-directory/another-file">
<TreeView.LeadingVisual>
<FileIcon />
</TreeView.LeadingVisual>
Another file
</TreeView.Item>
</TreeView.SubTree>
</TreeView.Item>
</TreeView.SubTree>
</TreeView.Item>
<TreeView.Item id="another-file" dragHandle={dragHandle}>
<TreeView.LeadingVisual>
<FileIcon />
</TreeView.LeadingVisual>
Another file
</TreeView.Item>
</TreeView>
</nav>
)
}

export const NestedScrollContainer: Story = () => {
return (
<Box sx={{maxHeight: '50vh', overflow: 'auto'}}>
Expand Down
13 changes: 13 additions & 0 deletions packages/react/src/TreeView/TreeView.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,19 @@ describe('Markup', () => {
const item1 = getByRole('treeitem', {name: /Item 1/})
expect(item1).toHaveFocus()
})

it("should have 'data-drag-and-drop' attribute when dragAndDrop is passed to TreeView", () => {
const {queryByRole} = renderWithTheme(
<TreeView aria-label="Test tree" dragAndDrop>
<TreeView.Item id="item-1">Item 1</TreeView.Item>
<TreeView.Item id="item-2">Item 2</TreeView.Item>
<TreeView.Item id="item-3">Item 3</TreeView.Item>
</TreeView>,
)

const root = queryByRole('tree')
expect(root).toHaveAttribute('data-drag-and-drop')
})
})

describe('Keyboard interactions', () => {
Expand Down
30 changes: 29 additions & 1 deletion packages/react/src/TreeView/TreeView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
FileDirectoryOpenFillIcon,
} from '@primer/octicons-react'
import clsx from 'clsx'
import React, {useCallback, useEffect} from 'react'
import React, {useCallback, useEffect, type ReactElement} from 'react'
import styled, {keyframes} from 'styled-components'
import {ConfirmationDialog} from '../ConfirmationDialog/ConfirmationDialog'
import Spinner from '../Spinner'
Expand Down Expand Up @@ -65,6 +65,7 @@ export type TreeViewProps = {
children: React.ReactNode
flat?: boolean
className?: string
dragAndDrop?: boolean
}

const UlBox = styled.ul<SxProp>`
Expand Down Expand Up @@ -120,6 +121,12 @@ const UlBox = styled.ul<SxProp>`
outline: 2px solid transparent;
outline-offset: -2px;
}

.PRIVATE_TreeView-item-drag-handle {
siddharthkp marked this conversation as resolved.
Show resolved Hide resolved
grid-area: drag;
height: 100%;
visibility: visible;
}
}

@media (pointer: coarse) {
Expand All @@ -141,6 +148,17 @@ const UlBox = styled.ul<SxProp>`
grid-template-columns: 0 0 1fr;
}

&[data-drag-and-drop='true'] .PRIVATE_TreeView-item-container {
grid-template-columns: 1.5rem calc(calc(var(--level) - 1) * (var(--toggle-width) / 2)) var(--toggle-width) 1fr;
grid-template-areas: 'drag spacer toggle content';
}

.PRIVATE_TreeView-item-drag-handle {
grid-area: drag;
height: 100%;
visibility: hidden;
}

.PRIVATE_TreeView-item[aria-current='true'] > .PRIVATE_TreeView-item-container {
background-color: ${get('colors.actionListItem.default.selectedBg')};

Expand Down Expand Up @@ -257,6 +275,7 @@ const Root: React.FC<TreeViewProps> = ({
children,
flat,
className,
dragAndDrop,
}) => {
const containerRef = React.useRef<HTMLUListElement>(null)
const mouseDownRef = React.useRef<boolean>(false)
Expand Down Expand Up @@ -312,6 +331,7 @@ const Root: React.FC<TreeViewProps> = ({
aria-label={ariaLabel}
aria-labelledby={ariaLabelledby}
data-omit-spacer={flat}
data-drag-and-drop={dragAndDrop}
onMouseDown={onMouseDown}
className={className}
>
Expand All @@ -337,6 +357,7 @@ export type TreeViewItemProps = {
onExpandedChange?: (expanded: boolean) => void
onSelect?: (event: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>) => void
className?: string
dragHandle?: ReactElement
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we need to require folks to pass dragHandle if they're passing dragAndDrop on the parent.

If we have a tree view where some nodes are draggable and others are not, we could assume all nodes are draggable by default. Then, we could add a prop to TreeView.Item that allows people to disable drag-and-drop functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to this one as well. I don't think we'll have a scenario where individual tree nodes would be prevented from being dragged and dropped?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we'll have a scenario where individual tree nodes would be prevented from being dragged and dropped?

@ericwbailey we could. If the user didn't have access to see an item, they wouldn't be able to drag it.

}

const Item = React.forwardRef<HTMLElement, TreeViewItemProps>(
Expand All @@ -351,6 +372,7 @@ const Item = React.forwardRef<HTMLElement, TreeViewItemProps>(
onSelect,
children,
className,
dragHandle,
},
ref,
) => {
Expand Down Expand Up @@ -488,6 +510,12 @@ const Item = React.forwardRef<HTMLElement, TreeViewItemProps>(
<div style={{gridArea: 'spacer', display: 'flex'}}>
<LevelIndicatorLines level={level} />
</div>
{dragHandle ? (
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
<div className="PRIVATE_TreeView-item-drag-handle" onMouseDown={() => setIsExpanded(false)}>
Copy link
Contributor

Choose a reason for hiding this comment

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

How would a keyboard user drag and drop?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can resolve this one for now, with this overall approach being understood.

Copy link
Contributor

Choose a reason for hiding this comment

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

After I interact with the drag handle, I can't get focus to go back into the tree view. Maybe I'm just doing something wrong - so you and other reviewers should check to make sure you can reproduce this bug.

Copy link
Member

Choose a reason for hiding this comment

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

Not just you, I can reproduce this as well. The focus shifts to body after interacting with the drag handle

focus.shifts.to.body.mov

{dragHandle}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should require folks to style their own drag handles. We should ship with a default recommended style.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to this idea. The internal consistency will help users understand what the UI does.

</div>
) : null}
{hasSubTree ? (
// This lint rule is disabled due to the guidelines in the `TreeView` api docs.
// https://github.com/github/primer/blob/main/apis/tree-view-api.md#the-expandcollapse-chevron-toggle
Expand Down
Loading