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.

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

Merged
merged 20 commits into from
May 15, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/tame-nails-live.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@primer/react": minor
"docs": patch
---

TreeView: Add support for `TreeView.LeadingAction`
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
33 changes: 33 additions & 0 deletions e2e/components/TreeView.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,4 +138,37 @@ test.describe('TreeView', () => {
})
}
})

test.describe('Leading Action', () => {
for (const theme of themes) {
test.describe(theme, () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-treeview-features--leading-action',
globals: {
colorScheme: theme,
},
})

expect(await page.screenshot()).toMatchSnapshot(`TreeView.Leading Action.${theme}.png`)
})

test('axe @aat', async ({page}) => {
await visit(page, {
id: 'components-treeview-features--leading-action',
globals: {
colorScheme: theme,
},
})
await expect(page).toHaveNoViolations({
rules: {
'color-contrast': {
enabled: theme !== 'dark_dimmed',
},
},
})
})
})
}
})
})
10 changes: 10 additions & 0 deletions packages/react/src/TreeView/TreeView.docs.json
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,16 @@
}
]
},
{
"name": "TreeView.LeadingAction",
"props": [
{
"name": "children",
"required": true,
"type": "React.ReactNode"
}
]
},
{
"name": "TreeView.DirectoryIcon",
"props": []
Expand Down
76 changes: 76 additions & 0 deletions packages/react/src/TreeView/TreeView.examples.stories.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import {GrabberIcon} from '@primer/octicons-react'
import type {Meta, Story} from '@storybook/react'
import React from 'react'
import Box from '../Box'
import {TreeView} from './TreeView'
import {IconButton} from '../Button'

const meta: Meta = {
title: 'Components/TreeView/Examples',
component: TreeView,
decorators: [
Story => {
return (
// Prevent TreeView from expanding to the full width of the screen
<Box sx={{maxWidth: 400}}>
<Story />
</Box>
)
},
],
}

export const DraggableListItem: Story = () => {
return (
<Box
sx={{
// using Box for css, this could be in a css file as well
'.treeview-item': {
'.treeview-leading-action': {visibility: 'hidden'},
'&:hover, &:focus': {
'.treeview-leading-action': {visibility: 'visible'},
},
},
}}
>
<TreeView aria-label="Issues">
<ControlledDraggableItem id="item-1">Item 1</ControlledDraggableItem>
<ControlledDraggableItem id="item-2">
Item 2
<TreeView.SubTree>
<TreeView.Item id="item-2-sub-task-1">sub task 1</TreeView.Item>
<TreeView.Item id="item-2-sub-task-2">sub task 2</TreeView.Item>
</TreeView.SubTree>
</ControlledDraggableItem>
<ControlledDraggableItem id="item-3">Item 3</ControlledDraggableItem>
</TreeView>
</Box>
)
}

const ControlledDraggableItem: React.FC<{id: string; children: React.ReactNode}> = ({id, children}) => {
const [expanded, setExpanded] = React.useState(false)

return (
<>
<TreeView.Item id={id} className="treeview-item" expanded={expanded} onExpandedChange={setExpanded}>
<TreeView.LeadingAction>
<IconButton
icon={GrabberIcon}
variant="invisible"
aria-label="Reorder item"
className="treeview-leading-action"
draggable="true"
onDragStart={() => {
setExpanded(false)
// other drag logic to follow
}}
/>
</TreeView.LeadingAction>
{children}
</TreeView.Item>
</>
)
}

export default meta
51 changes: 51 additions & 0 deletions packages/react/src/TreeView/TreeView.features.stories.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ import {
DiffRemovedIcon,
DiffRenamedIcon,
FileIcon,
GrabberIcon,
KebabHorizontalIcon,
IssueClosedIcon,
IssueOpenedIcon,
} from '@primer/octicons-react'
import type {Meta, Story} from '@storybook/react'
import React from 'react'
Expand Down Expand Up @@ -989,4 +992,52 @@ export const WithoutIndentation: Story = () => (
</nav>
)

export const LeadingAction: Story = () => {
return (
<TreeView aria-label="Issues">
<TreeView.Item id="item-0">
<TreeView.LeadingAction>
<IconButton icon={GrabberIcon} aria-label="Reorder item 1" variant="invisible" />
</TreeView.LeadingAction>
<TreeView.LeadingVisual>
<Octicon icon={IssueClosedIcon} sx={{color: 'done.fg'}} />
</TreeView.LeadingVisual>
Item 1
</TreeView.Item>
<TreeView.Item id="item-2">
<TreeView.LeadingAction>
<IconButton icon={GrabberIcon} aria-label="Reorder item 2" variant="invisible" />
</TreeView.LeadingAction>
<TreeView.LeadingVisual>
<Octicon icon={IssueOpenedIcon} sx={{color: 'open.fg'}} />
</TreeView.LeadingVisual>
Item 2
<TreeView.SubTree>
<TreeView.Item id="item-2-sub-task-1">
<TreeView.LeadingVisual>
<Octicon icon={IssueOpenedIcon} sx={{color: 'open.fg'}} />
</TreeView.LeadingVisual>
sub task 1
</TreeView.Item>
<TreeView.Item id="item-2-sub-task-2">
<TreeView.LeadingVisual>
<Octicon icon={IssueOpenedIcon} sx={{color: 'open.fg'}} />
</TreeView.LeadingVisual>
sub task 2
</TreeView.Item>
</TreeView.SubTree>
</TreeView.Item>
<TreeView.Item id="item-3">
<TreeView.LeadingAction>
<IconButton icon={GrabberIcon} aria-label="Reorder item 3" variant="invisible" />
</TreeView.LeadingAction>
<TreeView.LeadingVisual>
<Octicon icon={IssueOpenedIcon} sx={{color: 'open.fg'}} />
</TreeView.LeadingVisual>
Item 3
</TreeView.Item>
</TreeView>
)
}

export default meta
51 changes: 46 additions & 5 deletions packages/react/src/TreeView/TreeView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ const ItemContext = React.createContext<{
setIsExpanded: (isExpanded: boolean) => void
leadingVisualId: string
trailingVisualId: string
leadingActionId: string
Copy link
Member

Choose a reason for hiding this comment

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

@ayy-bc Don't think we need this anymore either, removed it :)

}>({
itemId: '',
level: 1,
Expand All @@ -54,6 +55,7 @@ const ItemContext = React.createContext<{
setIsExpanded: () => {},
leadingVisualId: '',
trailingVisualId: '',
leadingActionId: '',
})

// ----------------------------------------------------------------------------
Expand Down Expand Up @@ -97,15 +99,20 @@ const UlBox = styled.ul<SxProp>`
outline-offset: -2;
}
}
&[data-has-leading-action] {
--has-leading-action: 1;
}
}

.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';
--leading-action-width: calc(var(--has-leading-action, 0) * 1.5rem);
--spacer-width: calc(calc(var(--level) - 1) * (var(--toggle-width) / 2));
grid-template-columns: var(--spacer-width) var(--leading-action-width) var(--toggle-width) 1fr;
grid-template-areas: 'spacer leadingAction toggle content';
width: 100%;
min-height: 2rem; /* 32px */
font-size: ${get('fontSizes.1')};
Expand Down Expand Up @@ -138,7 +145,7 @@ const UlBox = styled.ul<SxProp>`
}

&[data-omit-spacer='true'] .PRIVATE_TreeView-item-container {
grid-template-columns: 0 0 1fr;
grid-template-columns: 0 0 0 1fr;
}

.PRIVATE_TreeView-item[aria-current='true'] > .PRIVATE_TreeView-item-container {
Expand Down Expand Up @@ -202,6 +209,12 @@ const UlBox = styled.ul<SxProp>`
color: ${get('colors.fg.muted')};
}

.PRIVATE_TreeView-item-leading-action {
display: flex;
color: ${get('colors.fg.muted')};
grid-area: leadingAction;
}

.PRIVATE_TreeView-item-level-line {
width: 100%;
height: 100%;
Expand Down Expand Up @@ -354,11 +367,16 @@ const Item = React.forwardRef<HTMLElement, TreeViewItemProps>(
},
ref,
) => {
const [slots, rest] = useSlots(children, {leadingVisual: LeadingVisual, trailingVisual: TrailingVisual})
const [slots, rest] = useSlots(children, {
leadingAction: LeadingAction,
leadingVisual: LeadingVisual,
trailingVisual: TrailingVisual,
})
const {expandedStateCache} = React.useContext(RootContext)
const labelId = useId()
const leadingVisualId = useId()
const trailingVisualId = useId()
const leadingActionId = useId()
const [isExpanded, setIsExpanded] = useControllableState({
name: itemId,
// If the item was previously mounted, it's expanded state might be cached.
Expand Down Expand Up @@ -434,6 +452,7 @@ const Item = React.forwardRef<HTMLElement, TreeViewItemProps>(
setIsExpanded: setIsExpandedWithCache,
leadingVisualId,
trailingVisualId,
leadingActionId,
}}
>
{/* @ts-ignore Box doesn't have type support for `ref` used in combination with `as` */}
Expand All @@ -444,11 +463,12 @@ const Item = React.forwardRef<HTMLElement, TreeViewItemProps>(
id={itemId}
role="treeitem"
aria-labelledby={labelId}
aria-describedby={`${leadingVisualId} ${trailingVisualId}`}
aria-describedby={`${leadingActionId} ${leadingVisualId} ${trailingVisualId}`}
Copy link
Member

Choose a reason for hiding this comment

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

Don't think the action should be used to describe the item, especially when the action itself is not accessible (aria-hidden + tab-index=-1) 🤔

cc @ericwbailey for confirmation

Copy link
Contributor

Choose a reason for hiding this comment

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

aria-hidden should hypothetically override this, but yes, I think we should remove it.

We'll likely want to include a hint down the line in the aria-label about state, such as "draggable". That's something we'll be researching with TetraLogical.

Copy link
Member

Choose a reason for hiding this comment

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

@ayy-bc Final thing for you to look at :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@siddharthkp I was just following what LeadingVisual and TrailingVisual in this case. Looking at it again, I think removing this makes sense and we can add the required labels to the components itself when we pass it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can remove it or you can do so, whatever you prefer (lmk).

Copy link
Member

Choose a reason for hiding this comment

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

@siddharthkp I was just following what LeadingVisual and TrailingVisual in this case. Looking at it again, I think removing this makes sense
I can remove it or you can do so, whatever you prefer (lmk).

Yes please, I think actions have to be treated differently than visuals

we can add the required labels to the components itself when we pass it.

Makes sense, but be careful about keyboard navigation. For example, the toggle element (picture below) is not accessible by keyboard and hence does not have a label and is aria-hidden

toggle element

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@siddharthkp pushed the changes

aria-level={level}
aria-expanded={isSubTreeEmpty ? undefined : isExpanded}
aria-current={isCurrentItem ? 'true' : undefined}
aria-selected={isFocused ? 'true' : 'false'}
data-has-leading-action={slots.leadingAction ? true : undefined}
onKeyDown={handleKeyDown}
onFocus={event => {
// Scroll the first child into view when the item receives focus
Expand Down Expand Up @@ -488,6 +508,7 @@ const Item = React.forwardRef<HTMLElement, TreeViewItemProps>(
<div style={{gridArea: 'spacer', display: 'flex'}}>
<LevelIndicatorLines level={level} />
</div>
{slots.leadingAction}
{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 Expand Up @@ -829,6 +850,25 @@ const TrailingVisual: React.FC<TreeViewVisualProps> = props => {

TrailingVisual.displayName = 'TreeView.TrailingVisual'

// ----------------------------------------------------------------------------
// TreeView.LeadingAction

const LeadingAction: React.FC<TreeViewVisualProps> = props => {
const {isExpanded, leadingActionId} = React.useContext(ItemContext)
const children = typeof props.children === 'function' ? props.children({isExpanded}) : props.children
return (
<>
<div className="PRIVATE_VisuallyHidden" aria-hidden={true} id={leadingActionId}>
{props.label}
</div>
<div className="PRIVATE_TreeView-item-leading-action" aria-hidden={true}>
{children}
</div>
</>
)
}

LeadingAction.displayName = 'TreeView.LeadingAction'
// ----------------------------------------------------------------------------
// TreeView.DirectoryIcon

Expand Down Expand Up @@ -898,6 +938,7 @@ ErrorDialog.displayName = 'TreeView.ErrorDialog'
export const TreeView = Object.assign(Root, {
Item,
SubTree,
LeadingAction,
LeadingVisual,
TrailingVisual,
DirectoryIcon,
Expand Down