Add `TreeView.LeadingAction` sub-component

[ayy-bc]

Closes #4542

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
• [x] 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

• [x] Added/updated tests
• [x] Added/updated documentation
• [x] Added/updated previews (Storybook)
• [x] Changes are SSR compatible
• [x] Tested in Chrome
• [x] Tested in Firefox
• [x] 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[bot]]

🦋 Changeset detected

Latest commit: 29348d64a079da59b80d7bb35bb2d4fecda85d57

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[bot]]

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% 🔺)

[ayy-bc]

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

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.

@ayy-bc @ericwbailey Can you say a little more about the demo purposes?

(If it's something urgent with user demos, we can ship this quickly as is and keep iterating on it. If it's internal demos not on production, when we can use a different method to unblock the team)

[siddharthkp]

Curious to hear your thoughts here @mperrotti,

This PR is very specific to drag and drop behavior, which is fine, as long as we also bake-in user feedback for dragging state.

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.

On a pattern level, however, the requirement/request from https://github.com/primer/react/issues/4542 and @ayy-bc's comment seems to be 2 things:

1. an icon button "slot" that is placed before both the expand/collapse button and the leading visual

   This feels similar to how we support both trailingVisual and trailingAction at the same time on components like Button, TextInput and PageHeader.

   Is this a good case for add a leadingAction slot along with the existing leadingVisual slot on TreeView.Item?

2. The ability to hide the button with css

   This part feels use case specific, so it would be better to put this at the usage site instead of baking it in the component.

   To be able to target the leadingAction slot, we would need a reliable selector like [data-component=TreeView.leadingAction]. (We do this in other components like Button, ActionList, UnderlineNav, etc. already)

@ayy-bc Does that seem correct or am I missing a crucial part here?

[ayy-bc]

@siddharthkp thanks for looking into this. We really appreciate it.

Can you say a little more about the demo purposes?

We are planning to ship the first iteration of drag and drop with sub-issues with our currently ongoing epic. The changes will be out in production for everyone flagged into sub-issues to try. Regarding the urgency, it would help us a ton to be able to ship this out soon as we're currently using a local copy of TreeView inside of nested list view to test out our changes.

Regarding your second comment:

1. Yes, that's spot on. We need an icon button slot before the expand/collapse toggle so we can place the drag handle there.
2. A little confused about this one. Ideally, the updated grid (with an area for leadingAction ~dragHandle) would apply to all the TreeView.Item's based on some prop/attribute (maybe at the parent TreeView level, and the updated grid with space for leading action is only used when the consumer passes some attribute/prop so as to not break other users of TreeView). Then, the user can pass the icon button (as leading action) to any TreeView.Item and for those that don't have the leading action passed, they would just show some empty space there so as to not break the tree layout (we'll only be rendering drag handles for first level items to begin with). I'm not the best at styling but if the end user/usage site can pass the leading action (~dragHandle) as a child to the TreeView.Item with the [data-component=TreeView.leadingAction] and if that places the handle in the right place, that should work with us

[ericwbailey]

@ayy-bc @ericwbailey Can you say a little more about the demo purposes?

(If it's something urgent with user demos, we can ship this quickly as is and keep iterating on it. If it's internal demos not on production, when we can use a different method to unblock the team)

Ah, sorry about that! I was referring to internal demos and the eventual adding of more Hubbers to the feature flag. So, no scary public-facing thing we need to rush to address.

---

For what it's worth, I really like the idea behind leadingAction. I also wonder if we'll run into a future state where the design calls for both drag and drop and a leading action?

[siddharthkp]

they would just show some empty space there so as to not break the tree layout (we'll only be rendering drag handles for first level items to begin with).

Ah, that's interesting. Would all items at the first level have a drag handle? (Is there a case where only some items have a drag handle, while others show empty space to keep alignment?)

[siddharthkp]

For what it's worth, I really like the idea behind leadingAction. I also wonder if we'll run into a future state where the design calls for both drag and drop and a leading action?

Definitely possible! Till now, the pattern has been to put action (or multiple actions) in trailing slots instead of leading

Regarding the urgency, it would help us a ton to be able to ship this out soon as we're currently using a local copy of TreeView inside of nested list view to test out our changes.

Understood. Also, that's a good workaround in the time being!

I'll wait for @mperrotti's thoughts on leadingAction before we conclude on the direction

[mperrotti]

I like the flexibility of a leadingAction, but I can't think of other possible leading actions since we typically put actions after the item label (in a "trailing" slot).

I feel comfortable moving forward with leadingAction so we can quickly unblock and iterate, ~but I don't think we need to have both leadingAction and leadingVisual yet~ Edit: we already have leadingVisual, so we should keep it.

[siddharthkp]

Great, we have a direction then!

@ayy-bc, I have an implementation in mind, I'll try to finish it tomorrow.

Do you mind if I update your PR itself with the changes? (because we have reviewers here already)

[ayy-bc]

@siddharthkp yeah please feel free to update this PR with the changes you have in mind. Thanks a ton for getting the ball rolling in this one :) I'll take a look at the PR whenever you have your changes in

[siddharthkp]

Pushed some changes, waiting for story to be deployed

Leaving notes for myself

TODOs In the story:

• [x] implement fold on click
• [x] implement hide until hovered

TODOs in the component:

• [ ] check if leading action should be aria-hidden
• [x] update snapshots
• [x] make the API always render an icon button

[siddharthkp]

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

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

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 :(

[ayy-bc]

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

[siddharthkp]

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