Add `TreeView.LeadingAction` sub-component #4546

ayy-bc · 2024-04-30T18:49:50Z

Changelog

This PR adds TreeView.LeadingAction sub component (like the leading visual, trailing visual) to create a slot for elements that go before the toggle and can be used for various use cases such as a drag handle.

Rollout strategy

Patch release
Minor release
Major release; if selected, include a written rollout or migration plan
None; if selected, include a brief description as to why

Testing & Reviewing

Merge checklist

Added/updated tests
Added/updated documentation
Added/updated previews (Storybook)
Changes are SSR compatible
Tested in Chrome
Tested in Firefox
Tested in Safari
Tested in Edge
(GitHub staff only) Integration tests pass at github/github (Learn more about how to run integration tests)

changeset-bot · 2024-04-30T18:49:54Z

🦋 Changeset detected

Latest commit: 29348d6

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2024-04-30T18:53:19Z

size-limit report 📦

Path	Size
packages/react/dist/browser.esm.js	88.45 KB (+0.12% 🔺)
packages/react/dist/browser.umd.js	88.77 KB (+0.14% 🔺)

packages/react/src/TreeView/TreeView.tsx

mperrotti

I have some initial feedback, but I really think we need somebody from a11y to take a look. For example, I'm not sure the "right"/accessible way to add interactive controls inside of a role="treeitem".

It also looks like consumers will have to write their own drag-and-drop functionality since there are no props to handle onDragStart/onDragEnd callbacks. Also, we should agree on the right designs/interactions for a tree view when it's actively being dragged. For example:

What does the node look like when the user is dragging it? Probably just knock the opacity down to make it look like a "ghost" of the original node.
How do we indicate the prospective new position of the node while it's being dragged? Probably a blue line that acts like a cursor to show what position the node will be in after it's dropped.

mperrotti · 2024-05-02T14:55:07Z

packages/react/src/TreeView/TreeView.features.stories.tsx

+ 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'
+ }


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

mperrotti · 2024-05-02T15:02:18Z

packages/react/src/TreeView/TreeView.tsx

+ {dragHandle ? (
+ // eslint-disable-next-line jsx-a11y/no-static-element-interactions
+ <div className="PRIVATE_TreeView-item-drag-handle" onMouseDown={() => setIsExpanded(false)}>
+ {dragHandle}


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

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

mperrotti · 2024-05-02T15:04:28Z

packages/react/src/TreeView/TreeView.tsx

@@ -337,6 +358,7 @@ export type TreeViewItemProps = {
 onExpandedChange?: (expanded: boolean) => void
 onSelect?: (event: React.MouseEvent<HTMLElement> | React.KeyboardEvent<HTMLElement>) => void
 className?: string
+ dragHandle?: ReactElement


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.

+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?

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.

mperrotti · 2024-05-02T15:05:45Z

packages/react/src/TreeView/TreeView.tsx

@@ -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)}>


How would a keyboard user drag and drop?

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

mperrotti · 2024-05-02T15:15:16Z

packages/react/src/TreeView/TreeView.tsx

@@ -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)}>


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.

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

ericwbailey

only show dragHandle when user hovers over TreeView.Item

One small thing I'd love to sneak in: can we also show the DragHandle via :focus-visible? That way it will also be shown to keyboard-only user.

I have some initial feedback, but I really think we need somebody from a11y to take a look. For example, I'm not sure the "right"/accessible way to add interactive controls inside of a role="treeitem".

This interaction pattern is really unique to the web, so it's going to take some time to research this. The other thought here is that my understanding of the shape of this work is getting initial functionality in for demo purposes, and then follow up with accessibility tweaks so it does not block the work.

With that context, I think we can consider this merge-able once other things folks reviewing have pointed out have been addressed. I simply don't know what will be needed yet. We have a plan in place for discovering that, but it will take time.

So, I'd consider this an approval from accessibility at a high level with the idea that there may need to be larger structural updates to follow as we learn what they'll be.

ayy-bc · 2024-05-02T18:45:57Z

@mperrotti @ericwbailey the initial idea was to just have some space before the expand/collapse toggle where users can place any element. I used dragHandle as the example as that is what we were interested in.

If we were to ship with a default recommended style to always have the dragHandle show, I believe the drag and drop logic should also reside within TreeView (at least for onDragStart/onDragEnd). This would complicate things quite a bit for our use-case.

I think the better way to put this is: do you have recommendations on what we could do if we just wanted to place an element before the TreeView.Item toggle? The consumer would handle what that element does (i.e clicking on it, dragging it, etc).

siddharthkp · 2024-05-13T14:04:57Z

@ericwbailey Do you know if should add an aria-hidden to the leading action until we know it's accessibility story? (There's no way to get to it with keyboard navigation either)

ericwbailey · 2024-05-13T14:11:42Z

@siddharthkp Yup, that'd be great. Ultimately, we'd need it and also tabindex="-1" applied to any interactive element on the leading action to fully suppress it. If you have the capacity to sneak that in now with this PR that'd be amazing!

siddharthkp · 2024-05-13T14:26:12Z

Ultimately, we'd need it and also tabindex="-1" applied to any interactive element on the leading action to fully suppress it. If you have the capacity to sneak that in now with this PR that'd be amazing!

Just noticed the changes made since my last review, looks like that would have to live in the application instead of primer/react because primer does not control the contents of the slot :(

siddharthkp · 2024-05-13T14:28:30Z

packages/react/src/TreeView/TreeView.tsx

@@ -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}`}


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

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.

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

@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.

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

@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

@siddharthkp pushed the changes

siddharthkp

Final conversation left to resolve and then we're good to go: f9b880d#r1598579584

ayy-bc · 2024-05-13T15:54:38Z

@siddharthkp replied to your comment above with some more context :)

siddharthkp · 2024-05-15T11:38:13Z

packages/react/src/TreeView/TreeView.tsx

@@ -45,6 +45,7 @@ const ItemContext = React.createContext<{
 setIsExpanded: (isExpanded: boolean) => void
 leadingVisualId: string
 trailingVisualId: string
+ leadingActionId: string


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

siddharthkp · 2024-05-15T12:20:06Z

@mperrotti Hi 👋 Can I get a review for the snapshots?

* add grid area for drag handle when data-drag-and-drop is true * add test to verify dnd attribute * css updates * add docs * test(vrt): update snapshots * Sid/treeview leading action (#4569) * wip: leading action slot * clean up a little * change from prop to subcomponent * remove outdated test * spacer should come before leadingAction * merge snapshots from main * merge package-lock from main * add visual tests * use IconButton for leadingAction * add example of drag handle on hover * Create tame-nails-live.md * test(vrt): update snapshots * change LeadingActio type to React.FC<TreeViewVisualProps> and accept children * change LeadingAction of type React.FC<TreeViewVisualProps> * typo * add `variant="invisible"` to icon button in stories * add leadingActionId and aria-hidden to LeadingAction subcomponent * remove `leadingActionId` to describe the tree view item * remove unused leadingActionId * remove docs from changeset --------- Co-authored-by: ayy-bc <[email protected]> Co-authored-by: Siddharth Kshetrapal <[email protected]> Co-authored-by: siddharthkp <[email protected]>

add grid area for drag handle when data-drag-and-drop is true

a5ccd2d

ayy-bc temporarily deployed to github-pages April 30, 2024 18:53 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4546 April 30, 2024 18:53 Inactive

add test to verify dnd attribute

66c876d

ayy-bc temporarily deployed to github-pages April 30, 2024 19:45 — with GitHub Actions Inactive

dusave reviewed Apr 30, 2024

View reviewed changes

packages/react/src/TreeView/TreeView.tsx Outdated Show resolved Hide resolved

github-actions bot temporarily deployed to storybook-preview-4546 April 30, 2024 19:45 Inactive

css updates

c982c1d

ayy-bc temporarily deployed to github-pages April 30, 2024 22:17 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4546 April 30, 2024 22:18 Inactive

add docs

61b2e50

ayy-bc temporarily deployed to github-pages April 30, 2024 23:23 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4546 April 30, 2024 23:23 Inactive

ayy-bc self-assigned this Apr 30, 2024

ayy-bc added the update snapshots label Apr 30, 2024

ayy-bc marked this pull request as ready for review April 30, 2024 23:37

ayy-bc requested a review from a team as a code owner April 30, 2024 23:37

ayy-bc requested a review from camertron April 30, 2024 23:37

test(vrt): update snapshots

bba15da

ayy-bc requested a review from a team as a code owner April 30, 2024 23:42

ayy-bc requested a review from langermank April 30, 2024 23:42

github-actions bot removed the update snapshots label Apr 30, 2024

primer bot temporarily deployed to github-pages April 30, 2024 23:45 Inactive

github-actions bot temporarily deployed to storybook-preview-4546 April 30, 2024 23:46 Inactive

mperrotti reviewed May 2, 2024

View reviewed changes

ericwbailey approved these changes May 2, 2024

View reviewed changes

JoseInTheArena removed the request for review from camertron May 3, 2024 16:47

ayy-bc temporarily deployed to github-pages May 9, 2024 19:08 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4546 May 9, 2024 19:09 Inactive

siddharthkp reviewed May 13, 2024

View reviewed changes

remove leadingActionId to describe the tree view item

3771156

ayy-bc temporarily deployed to github-pages May 14, 2024 17:00 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4546 May 14, 2024 17:01 Inactive

siddharthkp reviewed May 15, 2024

View reviewed changes

remove unused leadingActionId

c8b1086

siddharthkp temporarily deployed to github-pages May 15, 2024 11:44 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4546 May 15, 2024 11:44 Inactive

remove docs from changeset

b438d27

siddharthkp temporarily deployed to github-pages May 15, 2024 11:49 — with GitHub Actions Inactive

github-actions bot temporarily deployed to storybook-preview-4546 May 15, 2024 11:49 Inactive

Merge branch 'main' into ayy-bc/addTreeViewDragHandle

29348d6

siddharthkp deployed to github-pages May 15, 2024 12:00 — with GitHub Actions View deployment

github-actions bot deployed to storybook-preview-4546 May 15, 2024 12:00 View deployment

siddharthkp enabled auto-merge May 15, 2024 12:20

mperrotti approved these changes May 15, 2024

View reviewed changes

siddharthkp added this pull request to the merge queue May 15, 2024

Merged via the queue into main with commit c81898c May 15, 2024
30 checks passed

siddharthkp deleted the ayy-bc/addTreeViewDragHandle branch May 15, 2024 13:05

primer bot mentioned this pull request May 15, 2024

Release tracking #4595

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `TreeView.LeadingAction` sub-component #4546

Add `TreeView.LeadingAction` sub-component #4546

ayy-bc commented Apr 30, 2024 •

edited

changeset-bot bot commented Apr 30, 2024 •

edited

github-actions bot commented Apr 30, 2024 •

edited

mperrotti left a comment

mperrotti May 2, 2024

siddharthkp May 7, 2024

mperrotti May 2, 2024

ericwbailey May 2, 2024

mperrotti May 2, 2024

ericwbailey May 2, 2024

dusave May 7, 2024

mperrotti May 2, 2024

ericwbailey May 2, 2024

mperrotti May 2, 2024

siddharthkp May 6, 2024

ericwbailey left a comment

ayy-bc commented May 2, 2024

siddharthkp commented May 13, 2024

ericwbailey commented May 13, 2024 •

edited

siddharthkp commented May 13, 2024 •

edited

siddharthkp May 13, 2024

ericwbailey May 13, 2024

siddharthkp May 13, 2024

ayy-bc May 13, 2024

ayy-bc May 13, 2024

siddharthkp May 14, 2024 •

edited

ayy-bc May 14, 2024

siddharthkp left a comment

ayy-bc commented May 13, 2024

siddharthkp May 15, 2024 •

edited

siddharthkp commented May 15, 2024 •

edited

Add TreeView.LeadingAction sub-component #4546

Add TreeView.LeadingAction sub-component #4546

Conversation

ayy-bc commented Apr 30, 2024 • edited

Changelog

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Apr 30, 2024 • edited

🦋 Changeset detected

github-actions bot commented Apr 30, 2024 • edited

size-limit report 📦

mperrotti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericwbailey left a comment

Choose a reason for hiding this comment

ayy-bc commented May 2, 2024

siddharthkp commented May 13, 2024

ericwbailey commented May 13, 2024 • edited

siddharthkp commented May 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddharthkp May 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddharthkp left a comment

Choose a reason for hiding this comment

ayy-bc commented May 13, 2024

siddharthkp May 15, 2024 • edited

Choose a reason for hiding this comment

siddharthkp commented May 15, 2024 • edited

Add `TreeView.LeadingAction` sub-component #4546

Add `TreeView.LeadingAction` sub-component #4546

ayy-bc commented Apr 30, 2024 •

edited

changeset-bot bot commented Apr 30, 2024 •

edited

github-actions bot commented Apr 30, 2024 •

edited

ericwbailey commented May 13, 2024 •

edited

siddharthkp commented May 13, 2024 •

edited

siddharthkp May 14, 2024 •

edited

siddharthkp May 15, 2024 •

edited

siddharthkp commented May 15, 2024 •

edited