Skip to content

Commit

Permalink
fix(TreeView): Fix invalid tree item children (#1584)
Browse files Browse the repository at this point in the history
  • Loading branch information
@silvalaura
silvalaura authored Nov 21, 2024
1 parent c319a88 commit 46c70e6
Show file tree
Hide file tree
Showing 4 changed files with 259 additions and 24 deletions.
5 changes: 5 additions & 0 deletions .changeset/tree-validity.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'react-magma-dom': patch
---

fix(TreeView): Fix invalid tree item children
63 changes: 56 additions & 7 deletions packages/react-magma-dom/src/components/TreeView/TreeView.stories.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,10 @@ export const Complex = (args: Partial<TreeViewProps>) => {
);
const total = selectedItems?.length ?? 0;

const handleExpandedChange = (event: React.SyntheticEvent, expandedItems: string[]) => {
const handleExpandedChange = (
event: React.SyntheticEvent,
expandedItems: string[]
) => {
setExpandedItems(expandedItems);
};

Expand Down Expand Up @@ -358,8 +361,8 @@ export const Complex = (args: Partial<TreeViewProps>) => {
icon={<ArticleIcon aria-hidden={true} />}
label={
<>
Section 5.1.3.1: Apple pie apple pie tart macaroon topping
chocolate cake
Section 5.1.3.1: Apple pie apple pie tart macaroon
topping chocolate cake
</>
}
itemId="pt2ch5.1.3.1"
Expand All @@ -368,8 +371,8 @@ export const Complex = (args: Partial<TreeViewProps>) => {
icon={<ArticleIcon aria-hidden={true} />}
label={
<>
Section 5.1.3.2: Apple pie apple pie tart macaroon topping
chocolate cake
Section 5.1.3.2: Apple pie apple pie tart macaroon
topping chocolate cake
</>
}
itemId="pt2ch5.1.3.2"
Expand All @@ -378,8 +381,8 @@ export const Complex = (args: Partial<TreeViewProps>) => {
icon={<ArticleIcon aria-hidden={true} />}
label={
<>
Section 5.1.3.3: Apple pie apple pie tart macaroon topping
chocolate cake
Section 5.1.3.3: Apple pie apple pie tart macaroon
topping chocolate cake
</>
}
itemId="pt2ch5.1.3.3"
Expand Down Expand Up @@ -1038,3 +1041,49 @@ ParentsAndChildrenNotAutoChecked.parameters = {
exclude: ['isInverse', 'initialExpandedItems', 'ariaLabelledBy'],
},
};

export const InvalidTreeItems = (args: Partial<TreeViewProps>) => {
return (
<>
<p>
<em>
This is an example of a tree with badly structured tree items. Expect
only the following items to be expandable: Node 1, Child 1, Node 2,
Child 2, Grandchild 2.
</em>
</p>
<TreeView {...args}>
<TreeItem label="Node 0 - fragment" itemId="item0" testId="item0">
<></>
</TreeItem>
<TreeItem label="Node 1" itemId="item1" testId="item1">
<TreeItem label="Child 1" itemId="item-child1">
<TreeItem label="Grandchild 1" itemId="item-gchild1">
<Tag>This is a tag as a child of Grandchild 1</Tag>
</TreeItem>
</TreeItem>
</TreeItem>
<TreeItem label="Node 2" itemId="item2">
<TreeItem label="Child 2" itemId="item-child2">
<TreeItem label="Grandchild 2" itemId="item-gchild2">
<TreeItem label="Great-grandchild 2" itemId="item-ggchild2" />
<TreeItem label="Great-grandchild 3" itemId="item-ggchild3">
<>Invalid child</>
</TreeItem>
</TreeItem>
</TreeItem>
</TreeItem>
<TreeItem label="Node 3" itemId="item3"></TreeItem>
<TreeItem label="Node 4" itemId="item4">
Child of node 4 is just text
</TreeItem>
</TreeView>
</>
);
};

InvalidTreeItems.parameters = {
controls: {
exclude: ['isInverse', 'initialExpandedItems', 'ariaLabelledBy'],
},
};
158 changes: 158 additions & 0 deletions packages/react-magma-dom/src/components/TreeView/TreeView.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import userEvent from '@testing-library/user-event';
import { FavoriteIcon } from 'react-magma-icons';
import { transparentize } from 'polished';
import { IndeterminateCheckboxStatus } from '../IndeterminateCheckbox';
import { Tag } from '../Tag';
import { Paragraph } from '../Paragraph';

const TEXT = 'Test Text Tree Item';
const testId = 'tree-view';
Expand Down Expand Up @@ -2387,4 +2389,160 @@ describe('TreeView', () => {
expect(item2).toHaveAttribute('aria-expanded', 'false');
});
});

describe('tree validity', () => {
it('when a TreeView is passed as a child, the tree item is expandable', () => {
const { getByTestId } = render(
<TreeView>
<TreeItem label="Node 1" itemId="item1" testId="item1">
<TreeItem
label="Child 1"
itemId="item-child1"
testId="item-child1"
/>
</TreeItem>
</TreeView>
);

expect(getByTestId('item1-expand')).toBeInTheDocument();
});

it('when multiple TreeViews are passed as a child, the tree item is expandable', () => {
const { getByTestId } = render(
<TreeView>
<TreeItem label="Node 1" itemId="item1" testId="item1">
<TreeItem
label="Child 1"
itemId="item-child1"
testId="item-child1"
/>
<TreeItem
label="Child 2"
itemId="item-child2"
testId="item-child2"
/>
</TreeItem>
</TreeView>
);

expect(getByTestId('item1-expand')).toBeInTheDocument();
});

it('when multiple TreeViews with nested children are passed as a child, the tree items are expandable', () => {
const { getByTestId } = render(
<TreeView>
<TreeItem label="Node 1" itemId="item1" testId="item1">
<TreeItem
label="Child 1"
itemId="item-child1"
testId="item-child1"
/>
<TreeItem label="Child 2" itemId="item-child2" testId="item-child2">
<TreeItem
label="Child 2.1"
itemId="item-child2.1"
testId="item-child2.1"
>
<TreeItem
label="Child 2.1.1"
itemId="item-child2.1.1"
testId="item-child2.1.1"
/>
</TreeItem>
</TreeItem>
<TreeItem label="Child 3" itemId="item-child3" testId="item-child3">
<TreeItem
label="Child 3.1"
itemId="item-child3.1"
testId="item-child3.1"
/>
</TreeItem>
</TreeItem>
</TreeView>
);

expect(getByTestId('item1-expand')).toBeInTheDocument();
userEvent.click(getByTestId('item1-expand'));
expect(getByTestId('item-child2-expand')).toBeInTheDocument();
userEvent.click(getByTestId('item-child2-expand'));
expect(getByTestId('item-child2.1-expand')).toBeInTheDocument();
expect(getByTestId('item-child3-expand')).toBeInTheDocument();
});

it('when multiple TreeViews are passed as a child and at least one is valid, the tree item is expandable', () => {
const { getByTestId } = render(
<TreeView>
<TreeItem label="Node 1" itemId="item1" testId="item1">
<TreeItem label="Child 1" itemId="item-child1" testId="item-child1">
<></>
</TreeItem>
<TreeItem label="Child 2" itemId="item-child2" testId="item-child2">
<TreeItem
label="Child 2.1"
itemId="item-child2.1"
testId="item-child2.1"
/>
</TreeItem>
</TreeItem>
</TreeView>
);

expect(getByTestId('item1-expand')).toBeInTheDocument();
userEvent.click(getByTestId('item1-expand'));
expect(getByTestId('item-child2-expand')).toBeInTheDocument();
});


it('when a fragment is passed as a child, the tree item is not expandable', () => {
const { queryByTestId } = render(
<TreeView>
<TreeItem label="Node 1" itemId="item1" testId="item1">
<></>
</TreeItem>
<TreeItem label="Node 2" itemId="item2" testId="item2"></TreeItem>
</TreeView>
);

expect(queryByTestId('item1-expand')).not.toBeInTheDocument();
expect(queryByTestId('item2-expand')).not.toBeInTheDocument();
});

it('when any other component is passed as a child, the tree item is not expandable', () => {
const { queryByTestId } = render(
<TreeView>
<TreeItem label="Node 1" itemId="item1" testId="item1">
<Tag>This is a tag</Tag>
</TreeItem>
<TreeItem label="Node 2" itemId="item2" testId="item2">
<Paragraph>This is a paragraph</Paragraph>
</TreeItem>
</TreeView>
);

expect(queryByTestId('item1-expand')).not.toBeInTheDocument();
expect(queryByTestId('item2-expand')).not.toBeInTheDocument();
});

it('when text is passed as a child, the tree item is not expandable', () => {
const { queryByTestId } = render(
<TreeView>
<TreeItem label="Node 1" itemId="item1" testId="item1">
This is sample text
</TreeItem>
</TreeView>
);

expect(queryByTestId('item1-expand')).not.toBeInTheDocument();
});

it('when a TreeView does not have a child, the tree item is not expandable', () => {
const { queryByTestId } = render(
<TreeView>
<TreeItem label="Node 1" itemId="item1" testId="item1" />
</TreeView>
);

expect(queryByTestId('item1-expand')).not.toBeInTheDocument();
});
});
});
57 changes: 40 additions & 17 deletions packages/react-magma-dom/src/components/TreeView/utils.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,22 +152,6 @@ export function getChildrenItemIdsFlat(children) {
return itemIds;
}

// Return an array of objects where all children are items are nested in the parents
export function getChildrenItemIdsInTree(children) {
let itemIds = [];

React.Children.forEach(children, child => {
if (child.props?.itemId && !child.props?.isDisabled) {
itemIds.push({
itemId: child.props?.itemId,
children: getChildrenItemIdsInTree(child.props?.children),
});
}
});

return itemIds;
}

// Return a treeItemRefArray object with no null children
export function filterNullEntries(obj) {
if (Array.isArray(obj.current)) {
Expand Down Expand Up @@ -213,6 +197,45 @@ const getIsDisabled = ({
return isParentDisabled || isDisabled;
};

/* Returns a boolean indicating whether at least one child is valid.
A child is considered valid if it can be counted as an item that would make the parent expandable.
This is used to set `hasOwnTreeItems` which manages visibility of the expandable arrow.
*/
const areChildrenValid = children => {
if (!children) {
return false;
} else if (!children.length && children.type !== TreeItem) {
return false;
}

let hasValidChild = true;

for (let i = 0; i < children.length; i++) {
const child = children[i];

if (typeof child === 'string') {
return false; // Return false if a child is a string
}

if (child.type !== TreeItem) {
return hasValidChild;
}
// Recursively check the validity of nested children
if (child.props.children) {
const nestedChildren = Array.isArray(child.props.children)
? child.props.children
: [child.props.children];

if (areChildrenValid(nestedChildren)) {
hasValidChild = true;
return hasValidChild;
}
}
}

return hasValidChild;
};

const getTreeViewData = ({
children,
selectable,
Expand Down Expand Up @@ -249,7 +272,7 @@ const getTreeViewData = ({
itemId: props.itemId,
parentId,
icon: props.icon,
hasOwnTreeItems: Boolean(props.children),
hasOwnTreeItems: areChildrenValid(props.children),
isDisabled,
},
...(props.children
Expand Down

2 comments on commit 46c70e6

@github-actions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Deployed on https://next--upbeat-sinoussi-f675aa.netlify.app

@github-actions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀 Deployed on https://storybook-preview-dev--upbeat-sinoussi-f675aa.netlify.app

Please sign in to comment.