RAC Tree #5756

LFDanLu · 2024-01-26T18:09:45Z

Closes

✅ Pull Request Checklist:

Included link to corresponding React Spectrum GitHub Issue.
Added/updated unit tests and storybook for this change (for new code or code which already has tests).
Filled out test instructions.
Updated documentation (if it already exists for this component).
Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

…derProps functions

…start

…tence of content nodes

now that we are iterating over every tree item, we need to check if a rows parent is in the expanded key AND is also in the flattened row list. This is essentially checking if every parent above it is expanded as well since that is the only way its immediate parent would be in the flattened row list

…ridListItem this is because we need to stop the grid list keyboard navigation if we are going to perform an expand or collapse but cant do so since it is a capturing listener. An alternative is to copy all the code from useGridListItem as is to useTreeGridListItem but that can be discussed.

rspbot · 2024-01-27T00:15:25Z

Build successful! 🎉

rspbot · 2024-01-29T21:11:59Z

Build successful! 🎉

…nkaction

…start

LFDanLu · 2024-02-16T18:58:49Z

packages/react-aria-components/src/Tree.tsx

+        // TODO: missing selectionBehavior, hasAction and allowsSelection data attribute equivalents (available in renderProps). Do we want those?
+        data-expanded={hasChildRows ? isExpanded : undefined}


Table is similar here in that it doesn't have data attributes for the mentioned attributes which feels like it makes sense, not sure how helpful it would be to expose the above as data attributes

rspbot · 2024-02-16T19:13:36Z

Build successful! 🎉

rspbot · 2024-02-16T19:29:20Z

Build successful! 🎉

reidbarber

Looks great!

The only unexpected behavior I see is that typeahead has unpredictable behavior.

reidbarber · 2024-02-16T22:04:07Z

packages/react-aria-components/src/Tree.tsx

+ * and selection.
+ */
+const _Tree = /*#__PURE__*/ (forwardRef as forwardRefType)(Tree);
+export {_Tree as Tree};


Should this be called TreeView to better match the ARIA APG https://www.w3.org/WAI/ARIA/apg/patterns/treeview/

happy to change it, mainly was concerned about if the RSP tree would be called TreeView too haha. Anyone else have any thoughts? Maybe TreeGrid (except it is only one column...)

I think it's fine if they have the same name. IMO: TreeView > Tree > TreeGrid

packages/react-aria-components/src/Tree.tsx

Co-authored-by: Reid Barber <reid@reidbarber.com>

LFDanLu · 2024-02-16T22:32:49Z

@reidbarber what specifically did you run into for the typeahead?

reidbarber · 2024-02-16T22:38:00Z

@LFDanLu Looks like I was just pressing keys again before the cooldown period ended. No issues there, ignore that!

rspbot · 2024-02-16T22:41:43Z

Build successful! 🎉

snowystinger

first pass through tests

packages/react-aria-components/test/Tree.test.js

packages/react-aria-components/stories/Tree.stories.tsx

packages/react-aria-components/test/Tree.test.js

packages/@react-aria/gridlist/src/useGridListItem.ts

snowystinger · 2024-02-22T01:50:57Z

packages/@react-aria/gridlist/src/useGridListItem.ts

@@ -199,8 +246,9 @@ export function useGridListItem<T>(props: AriaGridListItemOptions, state: ListSt
    'aria-colindex': 1
  };

+  // TODO: should isExpanded and hasChildRows be a item state that gets returned by the hook?


seems useful to me

packages/@react-aria/tree/src/useTreeGridList.ts

packages/@react-aria/tree/src/useTreeGridListItem.ts

snowystinger · 2024-02-22T02:07:22Z

packages/@react-stately/tree/src/useTreeState.ts

+    // TODO: would be nice if the collection row information differentiated between childNodes vs childItems
+    // so we didn't have to keep iterating through info, perhaps make the user pass a prop to TreeItem for childItems/hasChildRows even in the static case?
+    updatedExpandedKeys = new Set([...collection].filter(row => {
+      return row.props.childItems || [...collection.getChildren(row.key)].filter(child => child.type === 'item').length > 0;


when you construct the collection, can you just mark the item node?

packages/react-aria-components/src/Tree.tsx

snowystinger · 2024-02-22T02:59:49Z

packages/react-aria-components/src/Tree.tsx

+// TODO does this need ref or context? Its only used to shallowly render the Content node... If it was a more generic collection component then I could see an argument for it
+// having those
+function TreeItemContent(props: TreeItemContentProps) {


what would the ref point to? there isn't an actual dom node corresponding to this right?

packages/react-aria-components/src/Tree.tsx

added test util and updating some tests for consistency

LFDanLu · 2024-02-22T19:56:59Z

packages/@react-aria/gridlist/src/useGridListItem.ts

+    if ('expandedKeys' in state && document.activeElement === ref.current) {
+      if ((e.key === EXPANSION_KEYS['expand'][direction]) && state.selectionManager.focusedKey === node.key && hasChildRows && state.expandedKeys !== 'all' && !state.expandedKeys.has(node.key)) {
+        state.toggleKey(node.key);
+        e.stopPropagation();
+        return;
+      } else if ((e.key === EXPANSION_KEYS['collapse'][direction]) && state.selectionManager.focusedKey === node.key && hasChildRows && (state.expandedKeys === 'all' || state.expandedKeys.has(node.key))) {
+        state.toggleKey(node.key);
+        e.stopPropagation();
+        return;
+      }
+    }
+


Need to modify the keyboard nav logic here for expansion and stuff since this is a capturing listener and thus can't be stopped in the useTreeGridListItem hook. If we want to keep this hooks clean from tree grid stuff, I can copy pasta the code here into useTreeGridListItem. Alternatively, I could make this hook and useGridList hook keep all the grid specific stuff and get rid of the TreeGridList hooks

rspbot · 2024-02-22T23:27:34Z

Build successful! 🎉

packages/dev/test-utils/src/events.ts

snowystinger

Giving preliminary approval. Is it ok to make the rest followup? I'd like to get this into testing.

reidbarber

LGTM, still have the question about naming, but we can change that later if we need to.

rspbot · 2024-03-01T18:42:27Z

Build successful! 🎉

rspbot · 2024-03-01T18:42:29Z

## API Changes

unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any', access: 'private' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }
unknown top level export { type: 'any' }

@react-aria/gridlist

useGridListItem

 useGridListItem<T> {
   props: AriaGridListItemOptions
-  state: ListState<T>
+  state: ListState<T> | TreeState<T>
   ref: RefObject<FocusableElement>
   returnVal: undefined
 }

@react-aria/tree

useTreeGridList

changed by:

AriaTreeGridListOptions
TreeGridListAria

-
+useTreeGridList<T> {
+  props: AriaTreeGridListOptions<T>
+  state: TreeState<T>
+  ref: RefObject<HTMLElement>
+  returnVal: undefined
+}

useTreeGridListItem

changed by:

AriaTreeGridListItemOptions
TreeGridListItemAria

-
+useTreeGridListItem<T> {
+  props: AriaTreeGridListItemOptions
+  state: TreeState<T>
+  ref: RefObject<FocusableElement>
+  returnVal: undefined
+}

AriaTreeGridListOptions

-
+AriaTreeGridListOptions<T> {
+  keyboardDelegate?: KeyboardDelegate
+}

it changed:

useTreeGridList

AriaTreeGridListProps

+AriaTreeGridListProps<T> {
 
+}

TreeGridListAria

-
+TreeGridListAria {
+  gridProps: DOMAttributes
+}

it changed:

useTreeGridList

TreeGridListProps

+TreeGridListProps<T> {
 
+}

AriaTreeGridListItemOptions

-
+AriaTreeGridListItemOptions {
+  node: Node<unknown>
+}

it changed:

useTreeGridListItem

TreeGridListItemAria

-
+TreeGridListItemAria {
+  descriptionProps: DOMAttributes
+  gridCellProps: DOMAttributes
+  rowProps: DOMAttributes
+}

it changed:

useTreeGridListItem

@react-stately/tree

TreeProps

 TreeProps<T> {
-
+  disabledBehavior?: DisabledBehavior
 }

it changed:

useTreeState

TreeState

 TreeState<T> {
   collection: Collection<Node<T>>
   disabledKeys: Set<Key>
-  expandedKeys: Set<Key>
+  expandedKeys: 'all' | Set<Key>
   selectionManager: SelectionManager
-  setExpandedKeys: (Set<Key>) => void
+  setExpandedKeys: ('all' | Set<Key>) => void
   toggleKey: (Key) => void
 }

it changed:

useTreeState

TreeCollection

 TreeCollection<T> {
   at: (number) => void
   constructor: (Iterable<Node<T>>, {
-    expandedKeys?: Set<Key>
+    expandedKeys?: 'all' | Set<Key>
 }) => void
   getFirstKey: () => void
   getItem: (Key) => void
   getKeyAfter: (Key) => void
   getKeys: () => void
   getLastKey: () => void
   size: any
   undefined: () => void
 }

LFDanLu · 2024-03-01T19:48:34Z

Merged for testing, feel free to continue reviewing off this PR if need be. I'll capture the followups in a ticket

devongovett · 2024-03-02T21:00:31Z

packages/@react-stately/tree/src/useTreeState.ts

@@ -26,13 +29,13 @@ export interface TreeState<T> {
  readonly disabledKeys: Set<Key>,

  /** A set of keys for items that are expanded. */
-  readonly expandedKeys: Set<Key>,
+  readonly expandedKeys: 'all' | Set<Key>,


changing this type could potentially be a breaking change. if someone was relying on expandedKeys being a set (and doing things like expandedKeys.has('foo') on it), then the 'all' value would cause an error to be thrown.

ah, good call, thats a bit annoying since it would be good for this to be consistent with our expandable rows table api... I can get rid of this 'all' support for now, but I guess I would have to create a separate tree state hook then to support 'all', thoughts?

where did the requirement for all come from? We don't have UI for an "expand all" button right?

we don't have a hard requirement for all but it was added to the expandable table rows to help with the eventual async case where a team may want all their rows to be auto expanded as they are loaded in

devongovett · 2024-03-02T21:09:19Z

packages/react-aria-components/src/Tree.tsx

+  let renderProps = useRenderProps({
+    ...props,
+    id: undefined,
+    children: item.rendered,


do tree items support render props? seems like that could conflict with the function for providing children?

oh hm, I hadn't really considered that, in my mind I figured users would use the renderProps that TreeItemContent provides. I'll see if I can get rid of the renderProps function for TreeItem entirely

Ah, actually I've already have the types setup such that the children of the TreeItem doesn't include the renderProps children definition. I've gotten rid of the ((item: T) => ReactElement) part of it since users should use a Collection for the dynamic case

LFDanLu added 23 commits January 8, 2024 15:44

start scaffolding

5a5364c

some planning

6aa1307

got the nested rendering to work

226470c

saving progress

c71e079

more musings

5ea4718

finally got static and dynamic working

e1477f8

tentative modifications to useTreeState and fix cases with static ren…

ff95af4

…derProps functions

updating so Content has renderProps instead

b893c15

Fix dynamic case

e8070ad

clean up

f07df3c

Merge branch 'main' of github.com:adobe/react-spectrum into treeview_…

ab88b79

…start

add treegridlist hooks and convert list stuff to use it

d7c242a

fix expanding and collapsing

8539dba

update the collection processing so that node index info ignores exis…

29aa9b4

…tence of content nodes

adding renderProps for expanded,level,etc and supporting empty state

a88f517

some cleanup

35e9ed4

fix TS strict

6f05b0f

add a checkbox to story and get rid of row double labelling

d9e85cd

update styles

460e577

fix some styling and lint

10aa9e7

forgot to make the tree package not private

87a901c

Merge branch 'main' into treeview_start

c1d645e

LFDanLu added 4 commits January 29, 2024 15:35

updating story as per testing review

a9057f5

add support for row expansion when clicking the row, onAction, and li…

06ef360

…nkaction

Merge branch 'main' of github.com:adobe/react-spectrum into treeview_…

8d3f442

…start

first batch of tests, basic interactions common to gridlist

e61dab1

LFDanLu commented Feb 16, 2024

View reviewed changes

LFDanLu changed the title ~~(WIP) RAC Tree~~ RAC Tree Feb 16, 2024

LFDanLu marked this pull request as ready for review February 16, 2024 19:15

Merge branch 'main' into treeview_start

9f43ecc

reidbarber reviewed Feb 16, 2024

View reviewed changes

Update packages/react-aria-components/src/Tree.tsx

aacd75d

Co-authored-by: Reid Barber <reid@reidbarber.com>

snowystinger reviewed Feb 22, 2024

View reviewed changes

LFDanLu added 2 commits February 22, 2024 11:48

addressing some review comments

7e11a92

added test util and updating some tests for consistency

forgot one review comment

27bf917

LFDanLu commented Feb 22, 2024

View reviewed changes

make test file tsx

040a34c

snowystinger reviewed Feb 26, 2024

View reviewed changes

packages/dev/test-utils/src/events.ts Show resolved Hide resolved

snowystinger approved these changes Mar 1, 2024

View reviewed changes

reidbarber approved these changes Mar 1, 2024

View reviewed changes

Merge branch 'main' into treeview_start

660b52d

LFDanLu merged commit 57ff24d into main Mar 1, 2024
27 checks passed

LFDanLu deleted the treeview_start branch March 1, 2024 19:47

devongovett reviewed Mar 2, 2024

View reviewed changes

LFDanLu mentioned this pull request Apr 10, 2024

v3 TreeView #6020

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RAC Tree #5756

RAC Tree #5756

LFDanLu commented Jan 26, 2024

rspbot commented Jan 27, 2024

rspbot commented Jan 29, 2024

LFDanLu Feb 16, 2024

rspbot commented Feb 16, 2024

rspbot commented Feb 16, 2024

reidbarber left a comment

reidbarber Feb 16, 2024

LFDanLu Feb 16, 2024

reidbarber Feb 17, 2024

LFDanLu commented Feb 16, 2024

reidbarber commented Feb 16, 2024

rspbot commented Feb 16, 2024

snowystinger left a comment

snowystinger Feb 22, 2024

snowystinger Feb 22, 2024

snowystinger Feb 22, 2024

LFDanLu Feb 22, 2024

rspbot commented Feb 22, 2024

snowystinger left a comment

reidbarber left a comment

rspbot commented Mar 1, 2024

rspbot commented Mar 1, 2024

LFDanLu commented Mar 1, 2024

devongovett Mar 2, 2024

LFDanLu Mar 4, 2024

devongovett Mar 4, 2024

LFDanLu Mar 4, 2024

devongovett Mar 2, 2024

LFDanLu Mar 4, 2024

LFDanLu Mar 4, 2024

		// TODO: missing selectionBehavior, hasAction and allowsSelection data attribute equivalents (available in renderProps). Do we want those?
		data-expanded={hasChildRows ? isExpanded : undefined}

RAC Tree #5756

RAC Tree #5756

Conversation

LFDanLu commented Jan 26, 2024

✅ Pull Request Checklist:

📝 Test Instructions:

🧢 Your Project:

rspbot commented Jan 27, 2024

rspbot commented Jan 29, 2024

Choose a reason for hiding this comment

rspbot commented Feb 16, 2024

rspbot commented Feb 16, 2024

reidbarber 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

LFDanLu commented Feb 16, 2024

reidbarber commented Feb 16, 2024

rspbot commented Feb 16, 2024

snowystinger 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

rspbot commented Feb 22, 2024

snowystinger left a comment

Choose a reason for hiding this comment

reidbarber left a comment

Choose a reason for hiding this comment

rspbot commented Mar 1, 2024

rspbot commented Mar 1, 2024

@react-aria/gridlist

useGridListItem

@react-aria/tree

useTreeGridList

useTreeGridListItem

AriaTreeGridListOptions

AriaTreeGridListProps

TreeGridListAria

TreeGridListProps

AriaTreeGridListItemOptions

TreeGridListItemAria

@react-stately/tree

TreeProps

TreeState

TreeCollection

LFDanLu commented Mar 1, 2024

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