-
Notifications
You must be signed in to change notification settings - Fork 520
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.
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
Add TreeView.LeadingAction
sub-component
#4546
Changes from 5 commits
a5ccd2d
66c876d
c982c1d
61b2e50
bba15da
1794aa5
5d54533
069b72a
dd39abb
f5e3dde
a8a3069
1f63a8b
f2442b7
920ea88
770e321
f9b880d
3771156
c8b1086
b438d27
29348d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' | ||
|
@@ -65,6 +65,7 @@ export type TreeViewProps = { | |
children: React.ReactNode | ||
flat?: boolean | ||
className?: string | ||
dragAndDrop?: boolean | ||
} | ||
|
||
const UlBox = styled.ul<SxProp>` | ||
|
@@ -120,6 +121,10 @@ 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
|
||
visibility: visible; | ||
} | ||
} | ||
|
||
@media (pointer: coarse) { | ||
|
@@ -141,6 +146,20 @@ 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; | ||
display: flex; | ||
align-items: center; | ||
justify-content: center; | ||
} | ||
|
||
.PRIVATE_TreeView-item[aria-current='true'] > .PRIVATE_TreeView-item-container { | ||
background-color: ${get('colors.actionListItem.default.selectedBg')}; | ||
|
||
|
@@ -257,6 +276,7 @@ const Root: React.FC<TreeViewProps> = ({ | |
children, | ||
flat, | ||
className, | ||
dragAndDrop, | ||
}) => { | ||
const containerRef = React.useRef<HTMLUListElement>(null) | ||
const mouseDownRef = React.useRef<boolean>(false) | ||
|
@@ -312,6 +332,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} | ||
> | ||
|
@@ -337,6 +358,7 @@ export type TreeViewItemProps = { | |
onExpandedChange?: (expanded: boolean) => void | ||
onSelect?: (event: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>) => void | ||
className?: string | ||
dragHandle?: ReactElement | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure we need to require folks to pass 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@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>( | ||
|
@@ -351,6 +373,7 @@ const Item = React.forwardRef<HTMLElement, TreeViewItemProps>( | |
onSelect, | ||
children, | ||
className, | ||
dragHandle, | ||
}, | ||
ref, | ||
) => { | ||
|
@@ -488,6 +511,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)}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would a keyboard user drag and drop? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1