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

Changes from 1 commit
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
14 changes: 10 additions & 4 deletions packages/react/src/TreeView/TreeView.tsx
View file
Open in desktop
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

@siddharthkp siddharthkp May 15, 2024 •
edited
Loading

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 @@ -374,6 +376,7 @@ const Item = React.forwardRef<HTMLElement, TreeViewItemProps>(
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 @@ -449,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 @@ -459,7 +463,7 @@ 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

@siddharthkp siddharthkp May 14, 2024 •
edited
Loading

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}
Expand Down Expand Up @@ -850,14 +854,16 @@ TrailingVisual.displayName = 'TreeView.TrailingVisual'
// TreeView.LeadingAction

const LeadingAction: React.FC<TreeViewVisualProps> = props => {
const {isExpanded, leadingVisualId} = React.useContext(ItemContext)
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={leadingVisualId}>
<div className="PRIVATE_VisuallyHidden" aria-hidden={true} id={leadingActionId}>
{props.label}
</div>
<div className="PRIVATE_TreeView-item-leading-action">{children}</div>
<div className="PRIVATE_TreeView-item-leading-action" aria-hidden={true}>
{children}
</div>
</>
)
}
Expand Down
Loading