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-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 <ayy-bc@users.noreply.github.com> Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com> Co-authored-by: siddharthkp <siddharthkp@users.noreply.github.com>

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 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 (36.19.0) #4595

Merged

This was referenced Jun 11, 2024

[Snyk] Upgrade @primer/react from 36.18.0 to 36.19.0 JonathanXDR/Atom#27

Closed

[Snyk] Upgrade @primer/react from 36.18.0 to 36.19.1 JonathanXDR/Atom#29

Closed

OATEF4313 mentioned this pull request Jun 20, 2024

[Snyk] Upgrade @primer/react from 36.19.0 to 36.19.1 OATEF4313/docs#3

Open

This was referenced Jun 20, 2024

[Snyk] Upgrade @primer/react from 36.18.0 to 36.19.1 JonathanXDR/Atom#30

Closed

[Snyk] Upgrade @primer/react from 36.18.0 to 36.20.0 JonathanXDR/Atom#31

Closed

StemmlerSisters mentioned this pull request Jul 16, 2024

[Snyk] Upgrade @primer/react from 36.1.0 to 36.23.0 StemmlerSisters/documentation#5

Open

snyk-io bot mentioned this pull request Jul 17, 2024

[Snyk] Upgrade @primer/react from 36.11.0 to 36.23.0 Hawthorne001/documentation.npm#3

Open

julitabarelkowska mentioned this pull request Jul 17, 2024

[Snyk] Upgrade @primer/react from 36.11.0 to 36.23.0 julitabarelkowska/documentation#7

Open

Canadatechmanufacturing mentioned this pull request Jul 17, 2024

[Snyk] Upgrade @primer/react from 36.7.0 to 36.23.0 Canadatechmanufacturing/documentation#3

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

Loading

changeset-bot bot commented Apr 30, 2024 •

edited

Loading

github-actions bot commented Apr 30, 2024 •

edited

Loading

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 May 15, 2024 •

edited

Loading

siddharthkp commented May 15, 2024 •

edited

Loading

Add TreeView.LeadingAction sub-component #4546

Add TreeView.LeadingAction sub-component #4546

Conversation

ayy-bc commented Apr 30, 2024 • edited Loading

Changelog

Rollout strategy

Testing & Reviewing

Merge checklist

changeset-bot bot commented Apr 30, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Apr 30, 2024 • edited Loading

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 May 15, 2024 • edited Loading

Choose a reason for hiding this comment

siddharthkp commented May 15, 2024 • edited Loading

Add `TreeView.LeadingAction` sub-component #4546

Add `TreeView.LeadingAction` sub-component #4546

ayy-bc commented Apr 30, 2024 •

edited

Loading

changeset-bot bot commented Apr 30, 2024 •

edited

Loading

github-actions bot commented Apr 30, 2024 •

edited

Loading

siddharthkp May 15, 2024 •

edited

Loading

siddharthkp commented May 15, 2024 •

edited

Loading