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

fix: S2 treeview haschildnodes #7694

Merged
merged 2 commits into from
Feb 11, 2025
Merged

fix: S2 treeview haschildnodes #7694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
54c44fe
fix logic to determine hasChildNodes
snowystinger Jan 31, 2025
afe91b8
revert breaking changes but use prop override
snowystinger Feb 11, 2025
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
7 changes: 5 additions & 2 deletions packages/@react-aria/gridlist/src/useGridListItem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,9 @@ export interface AriaGridListItemOptions {
/** Whether the list row is contained in a virtual scroller. */
isVirtualized?: boolean,
/** Whether selection should occur on press up instead of press down. */
shouldSelectOnPressUp?: boolean
shouldSelectOnPressUp?: boolean,
/** Whether this item has children, even if not loaded yet. */
hasChildItems?: boolean
}

export interface GridListItemAria extends SelectableItemStates {
Expand Down Expand Up @@ -86,13 +88,14 @@ export function useGridListItem<T>(props: AriaGridListItemOptions, state: ListSt
};

let treeGridRowProps: HTMLAttributes<HTMLElement> = {};
let hasChildRows;
let hasChildRows = props.hasChildItems;
let hasLink = state.selectionManager.isLink(node.key);
if (node != null && 'expandedKeys' in state) {
// TODO: ideally node.hasChildNodes would be a way to tell if a row has child nodes, but the row's contents make it so that value is always
// true...
let children = state.collection.getChildren?.(node.key);
hasChildRows = [...(children ?? [])].length > 1;

if (onAction == null && !hasLink && state.selectionManager.selectionMode === 'none' && hasChildRows) {
onAction = () => state.toggleKey(node.key);
}
Expand Down
25 changes: 9 additions & 16 deletions packages/@react-spectrum/s2/src/TreeView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,24 +294,19 @@ const treeRowFocusIndicator = raw(`
}`
);

let TreeItemContext = createContext<{hasChildItems?: boolean}>({});

export const TreeViewItem = <T extends object>(props: TreeViewItemProps<T>) => {
let {
href
} = props;
let {isDetached, isEmphasized} = useContext(InternalTreeContext);

return (
// TODO right now all the tree rows have the various data attributes applied on their dom nodes, should they? Doesn't feel very useful
<TreeItemContext.Provider value={{hasChildItems: !!props.childItems}}>
<UNSTABLE_TreeItem
{...props}
className={(renderProps) => treeRow({
...renderProps,
isLink: !!href, isEmphasized
}) + (renderProps.isFocusVisible && !isDetached ? ' ' + treeRowFocusIndicator : '')} />
</TreeItemContext.Provider>
<UNSTABLE_TreeItem
{...props}
className={(renderProps) => treeRow({
...renderProps,
isLink: !!href, isEmphasized
}) + (renderProps.isFocusVisible && !isDetached ? ' ' + treeRowFocusIndicator : '')} />
);
};

Expand All @@ -320,11 +315,10 @@ export const TreeItemContent = (props: Omit<TreeItemContentProps, 'children'> &
children
} = props;
let {isDetached, isEmphasized} = useContext(InternalTreeContext);
let {hasChildItems} = useContext(TreeItemContext);

return (
<UNSTABLE_TreeItemContent>
{({isExpanded, hasChildRows, selectionMode, selectionBehavior, isDisabled, isFocusVisible, isSelected, id, state}) => {
{({isExpanded, hasChildItems, selectionMode, selectionBehavior, isDisabled, isFocusVisible, isSelected, id, state}) => {
let isNextSelected = false;
let isNextFocused = false;
let keyAfter = state.collection.getKeyAfter(id);
Expand All @@ -348,7 +342,7 @@ export const TreeItemContent = (props: Omit<TreeItemContentProps, 'children'> &
width: '[calc(calc(var(--tree-item-level, 0) - 1) * var(--indent))]'
})} />
{/* TODO: revisit when we do async loading, at the moment hasChildItems will only cause the chevron to be rendered, no aria/data attributes indicating the row's expandability are added */}
{(hasChildRows || hasChildItems) && <ExpandableRowChevron isDisabled={isDisabled} isExpanded={isExpanded} />}
{hasChildItems && <ExpandableRowChevron isDisabled={isDisabled} isExpanded={isExpanded} />}
<Provider
values={[
[TextContext, {styles: treeContent}],
Expand All @@ -357,8 +351,7 @@ export const TreeItemContent = (props: Omit<TreeItemContentProps, 'children'> &
styles: style({size: fontRelative(20), flexShrink: 0})
}],
[ActionButtonGroupContext, {styles: treeActions}],
[ActionMenuContext, {styles: treeActionMenu, isQuiet: true, 'aria-label': 'Actions'}],
[TreeItemContext, {}]
[ActionMenuContext, {styles: treeActionMenu, isQuiet: true, 'aria-label': 'Actions'}]
]}>
{children}
</Provider>
Expand Down
30 changes: 10 additions & 20 deletions packages/@react-spectrum/tree/src/TreeView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import ChevronLeftMedium from '@spectrum-icons/ui/ChevronLeftMedium';
import ChevronRightMedium from '@spectrum-icons/ui/ChevronRightMedium';
import {DOMRef, Expandable, Key, SelectionBehavior, SpectrumSelectionProps, StyleProps} from '@react-types/shared';
import {isAndroid} from '@react-aria/utils';
import React, {createContext, JSX, JSXElementConstructor, ReactElement, ReactNode, useContext, useRef} from 'react';
import React, {createContext, JSX, JSXElementConstructor, ReactElement, ReactNode, useRef} from 'react';
import {SlotProvider, useDOMRef, useStyleProps} from '@react-spectrum/utils';
import {style} from '@react-spectrum/style-macro-s1' with {type: 'macro'};
import {useButton} from '@react-aria/button';
Expand All @@ -40,8 +40,6 @@ export interface SpectrumTreeViewProps<T> extends Omit<AriaTreeGridListProps<T>,
export interface SpectrumTreeViewItemProps<T extends object = object> extends Omit<TreeItemProps, 'className' | 'style' | 'value' | 'onHoverStart' | 'onHoverEnd' | 'onHoverChange'> {
/** Rendered contents of the tree item or child items. */
children: ReactNode,
/** Whether this item has children, even if not loaded yet. */
hasChildItems?: boolean,
/** A list of child tree item objects used when dynamically rendering the tree item children. */
childItems?: Iterable<T>
}
Expand Down Expand Up @@ -219,23 +217,18 @@ const treeRowOutline = style({
}
});

let TreeItemContext = createContext<{hasChildItems?: boolean}>({});

export const TreeViewItem = <T extends object>(props: SpectrumTreeViewItemProps<T>) => {
let {
href
} = props;

return (
// TODO right now all the tree rows have the various data attributes applied on their dom nodes, should they? Doesn't feel very useful
<TreeItemContext.Provider value={{hasChildItems: !!props.childItems || props.hasChildItems}}>
<UNSTABLE_TreeItem
{...props}
className={renderProps => treeRow({
...renderProps,
isLink: !!href
})} />
</TreeItemContext.Provider>
<UNSTABLE_TreeItem
{...props}
className={renderProps => treeRow({
...renderProps,
isLink: !!href
})} />
);
};

Expand All @@ -244,11 +237,10 @@ export const TreeItemContent = (props: Omit<TreeItemContentProps, 'children'> &
let {
children
} = props;
let {hasChildItems} = useContext(TreeItemContext);

return (
<UNSTABLE_TreeItemContent>
{({isExpanded, hasChildRows, level, selectionMode, selectionBehavior, isDisabled, isSelected, isFocusVisible}) => (
{({isExpanded, hasChildItems, level, selectionMode, selectionBehavior, isDisabled, isSelected, isFocusVisible}) => (
<div className={treeCellGrid({isDisabled})}>
{selectionMode !== 'none' && selectionBehavior === 'toggle' && (
// TODO: add transition?
Expand All @@ -260,7 +252,7 @@ export const TreeItemContent = (props: Omit<TreeItemContentProps, 'children'> &
)}
<div style={{gridArea: 'level-padding', marginInlineEnd: `calc(${level - 1} * var(--spectrum-global-dimension-size-200))`}} />
{/* TODO: revisit when we do async loading, at the moment hasChildItems will only cause the chevron to be rendered, no aria/data attributes indicating the row's expandability are added */}
{(hasChildRows || hasChildItems) && <ExpandableRowChevron isDisabled={isDisabled} isExpanded={isExpanded} />}
{hasChildItems && <ExpandableRowChevron isDisabled={isDisabled} isExpanded={isExpanded} />}
<SlotProvider
slots={{
text: {UNSAFE_className: treeContent({isDisabled})},
Expand All @@ -278,9 +270,7 @@ export const TreeItemContent = (props: Omit<TreeItemContentProps, 'children'> &
},
actionMenu: {UNSAFE_className: treeActionMenu(), UNSAFE_style: {marginInlineEnd: '.5rem'}, isQuiet: true}
}}>
<TreeItemContext.Provider value={{}}>
{children}
</TreeItemContext.Provider>
{children}
</SlotProvider>
<div className={treeRowOutline({isFocusVisible, isSelected})} />
</div>
Expand Down
38 changes: 19 additions & 19 deletions packages/@react-spectrum/tree/test/TreeView.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,7 @@ describe('Tree', () => {
expect(rowNoChild).toHaveAttribute('data-level', '1');
expect(rowNoChild).toHaveAttribute('aria-posinset', '1');
expect(rowNoChild).toHaveAttribute('aria-setsize', '2');
expect(rowNoChild).not.toHaveAttribute('data-has-child-rows');
expect(rowNoChild).not.toHaveAttribute('data-has-child-items');
expect(rowNoChild).toHaveAttribute('data-rac');

let rowWithChildren = rows[1];
Expand All @@ -305,7 +305,7 @@ describe('Tree', () => {
expect(rowWithChildren).toHaveAttribute('data-level', '1');
expect(rowWithChildren).toHaveAttribute('aria-posinset', '2');
expect(rowWithChildren).toHaveAttribute('aria-setsize', '2');
expect(rowWithChildren).toHaveAttribute('data-has-child-rows', 'true');
expect(rowWithChildren).toHaveAttribute('data-has-child-items', 'true');
expect(rowWithChildren).toHaveAttribute('data-rac');

let level2ChildRow = rows[2];
Expand All @@ -316,7 +316,7 @@ describe('Tree', () => {
expect(level2ChildRow).toHaveAttribute('data-level', '2');
expect(level2ChildRow).toHaveAttribute('aria-posinset', '1');
expect(level2ChildRow).toHaveAttribute('aria-setsize', '3');
expect(level2ChildRow).toHaveAttribute('data-has-child-rows', 'true');
expect(level2ChildRow).toHaveAttribute('data-has-child-items', 'true');
expect(level2ChildRow).toHaveAttribute('data-rac');

let level3ChildRow = rows[3];
Expand All @@ -327,7 +327,7 @@ describe('Tree', () => {
expect(level3ChildRow).toHaveAttribute('data-level', '3');
expect(level3ChildRow).toHaveAttribute('aria-posinset', '1');
expect(level3ChildRow).toHaveAttribute('aria-setsize', '1');
expect(level3ChildRow).not.toHaveAttribute('data-has-child-rows');
expect(level3ChildRow).not.toHaveAttribute('data-has-child-items');
expect(level3ChildRow).toHaveAttribute('data-rac');

let level2ChildRow2 = rows[4];
Expand All @@ -338,7 +338,7 @@ describe('Tree', () => {
expect(level2ChildRow2).toHaveAttribute('data-level', '2');
expect(level2ChildRow2).toHaveAttribute('aria-posinset', '2');
expect(level2ChildRow2).toHaveAttribute('aria-setsize', '3');
expect(level2ChildRow2).not.toHaveAttribute('data-has-child-rows');
expect(level2ChildRow2).not.toHaveAttribute('data-has-child-items');
expect(level2ChildRow2).toHaveAttribute('data-rac');

let level2ChildRow3 = rows[5];
Expand All @@ -349,7 +349,7 @@ describe('Tree', () => {
expect(level2ChildRow3).toHaveAttribute('data-level', '2');
expect(level2ChildRow3).toHaveAttribute('aria-posinset', '3');
expect(level2ChildRow3).toHaveAttribute('aria-setsize', '3');
expect(level2ChildRow3).not.toHaveAttribute('data-has-child-rows');
expect(level2ChildRow3).not.toHaveAttribute('data-has-child-items');
expect(level2ChildRow3).toHaveAttribute('data-rac');
});

Expand All @@ -358,7 +358,7 @@ describe('Tree', () => {

let rows = getAllByRole('row');
expect(rows[1]).toHaveAttribute('aria-label', 'Projects');
expect(rows[1]).toHaveAttribute('data-has-child-rows', 'true');
expect(rows[1]).toHaveAttribute('data-has-child-items', 'true');
expect(rows[1]).toHaveAttribute('aria-selected', 'false');
});

Expand All @@ -374,28 +374,28 @@ describe('Tree', () => {
expect(rows[0]).toHaveAttribute('aria-level', '1');
expect(rows[0]).toHaveAttribute('aria-posinset', '1');
expect(rows[0]).toHaveAttribute('aria-setsize', '2');
expect(rows[0]).toHaveAttribute('data-has-child-rows', 'true');
expect(rows[0]).toHaveAttribute('data-has-child-items', 'true');

expect(rows[2]).toHaveAttribute('aria-label', 'Project 2');
expect(rows[2]).toHaveAttribute('aria-expanded', 'true');
expect(rows[2]).toHaveAttribute('aria-level', '2');
expect(rows[2]).toHaveAttribute('aria-posinset', '2');
expect(rows[2]).toHaveAttribute('aria-setsize', '5');
expect(rows[2]).toHaveAttribute('data-has-child-rows', 'true');
expect(rows[2]).toHaveAttribute('data-has-child-items', 'true');

expect(rows[8]).toHaveAttribute('aria-label', 'Project 5');
expect(rows[8]).toHaveAttribute('aria-expanded', 'true');
expect(rows[8]).toHaveAttribute('aria-level', '2');
expect(rows[8]).toHaveAttribute('aria-posinset', '5');
expect(rows[8]).toHaveAttribute('aria-setsize', '5');
expect(rows[8]).toHaveAttribute('data-has-child-rows', 'true');
expect(rows[8]).toHaveAttribute('data-has-child-items', 'true');

expect(rows[12]).toHaveAttribute('aria-label', 'Reports');
expect(rows[12]).toHaveAttribute('aria-expanded', 'true');
expect(rows[12]).toHaveAttribute('aria-level', '1');
expect(rows[12]).toHaveAttribute('aria-posinset', '2');
expect(rows[12]).toHaveAttribute('aria-setsize', '2');
expect(rows[12]).toHaveAttribute('data-has-child-rows', 'true');
expect(rows[12]).toHaveAttribute('data-has-child-items', 'true');

expect(rows[16]).toHaveAttribute('aria-label', 'Reports 1ABC');
expect(rows[16]).toHaveAttribute('aria-level', '5');
Expand Down Expand Up @@ -463,7 +463,7 @@ describe('Tree', () => {
expect(treeTester.selectedRows[0]).toBe(row1);
});

it('should render a chevron for an expandable row marked with hasChildRows', () => {
it('should render a chevron for an expandable row marked with hasChildItems', () => {
let {getAllByRole} = render(
<TreeView aria-label="test tree">
<TreeViewItem textValue="Test" hasChildItems>
Expand All @@ -480,7 +480,7 @@ describe('Tree', () => {
expect(rows[0]).toHaveAttribute('aria-label', 'Test');
// Until the row gets children, don't mark it with the aria/data attributes.
expect(rows[0]).not.toHaveAttribute('aria-expanded');
expect(rows[0]).not.toHaveAttribute('data-has-child-rows');
expect(rows[0]).toHaveAttribute('data-has-child-items');
expect(chevron).toBeTruthy();
});

Expand Down Expand Up @@ -893,7 +893,7 @@ describe('Tree', () => {
expect(rows[0]).toHaveAttribute('aria-level', '1');
expect(rows[0]).toHaveAttribute('aria-posinset', '1');
expect(rows[0]).toHaveAttribute('aria-setsize', '2');
expect(rows[0]).toHaveAttribute('data-has-child-rows', 'true');
expect(rows[0]).toHaveAttribute('data-has-child-items', 'true');
expect(onExpandedChange).toHaveBeenCalledTimes(0);

// Check we can open/close a top level row
Expand All @@ -904,7 +904,7 @@ describe('Tree', () => {
expect(rows[0]).toHaveAttribute('aria-level', '1');
expect(rows[0]).toHaveAttribute('aria-posinset', '1');
expect(rows[0]).toHaveAttribute('aria-setsize', '2');
expect(rows[0]).toHaveAttribute('data-has-child-rows', 'true');
expect(rows[0]).toHaveAttribute('data-has-child-items', 'true');
expect(onExpandedChange).toHaveBeenCalledTimes(1);
// Note that the children of the parent row will still be in the "expanded" array
expect(new Set(onExpandedChange.mock.calls[0][0])).toEqual(new Set(['Project-2', 'Project-5', 'Reports', 'Reports-1', 'Reports-1A', 'Reports-1AB']));
Expand All @@ -918,7 +918,7 @@ describe('Tree', () => {
expect(rows[0]).toHaveAttribute('aria-level', '1');
expect(rows[0]).toHaveAttribute('aria-posinset', '1');
expect(rows[0]).toHaveAttribute('aria-setsize', '2');
expect(rows[0]).toHaveAttribute('data-has-child-rows', 'true');
expect(rows[0]).toHaveAttribute('data-has-child-items', 'true');
expect(onExpandedChange).toHaveBeenCalledTimes(2);
expect(new Set(onExpandedChange.mock.calls[1][0])).toEqual(new Set(['Projects', 'Project-2', 'Project-5', 'Reports', 'Reports-1', 'Reports-1A', 'Reports-1AB']));
rows = treeTester.rows;
Expand All @@ -932,7 +932,7 @@ describe('Tree', () => {
expect(rows[2]).toHaveAttribute('aria-level', '2');
expect(rows[2]).toHaveAttribute('aria-posinset', '2');
expect(rows[2]).toHaveAttribute('aria-setsize', '5');
expect(rows[2]).toHaveAttribute('data-has-child-rows', 'true');
expect(rows[2]).toHaveAttribute('data-has-child-items', 'true');

// Check we can close a nested row and it doesn't affect the parent
await treeTester.toggleRowExpansion({row: rows[2], interactionType: type as 'mouse' | 'keyboard'});
Expand All @@ -942,13 +942,13 @@ describe('Tree', () => {
expect(rows[2]).toHaveAttribute('aria-level', '2');
expect(rows[2]).toHaveAttribute('aria-posinset', '2');
expect(rows[2]).toHaveAttribute('aria-setsize', '5');
expect(rows[2]).toHaveAttribute('data-has-child-rows', 'true');
expect(rows[2]).toHaveAttribute('data-has-child-items', 'true');
expect(rows[0]).toHaveAttribute('aria-expanded', 'true');
expect(rows[0]).toHaveAttribute('data-expanded', 'true');
expect(rows[0]).toHaveAttribute('aria-level', '1');
expect(rows[0]).toHaveAttribute('aria-posinset', '1');
expect(rows[0]).toHaveAttribute('aria-setsize', '2');
expect(rows[0]).toHaveAttribute('data-has-child-rows', 'true');
expect(rows[0]).toHaveAttribute('data-has-child-items', 'true');
expect(onExpandedChange).toHaveBeenCalledTimes(3);
expect(new Set(onExpandedChange.mock.calls[2][0])).toEqual(new Set(['Projects', 'Project-5', 'Reports', 'Reports-1', 'Reports-1A', 'Reports-1AB']));
rows = treeTester.rows;
Expand Down
2 changes: 1 addition & 1 deletion packages/react-aria-components/docs/Tree.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ let items = [
position: relative;
transform: translateZ(0);

&[data-has-child-rows] {
&[data-has-child-items] {
--padding: 0px;
}

Expand Down
Loading