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

[TreeItem] allow conditional child #20154 #20156

Closed
wants to merge 5 commits into from
Closed

[TreeItem] allow conditional child #20154 #20156

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b9073e4
[TreeItem] allow conditional child
tonyhallett Mar 17, 2020
b1c68da
explicitly find conditional TreeItem child
tonyhallett Mar 17, 2020
3d0cea7
prettier
tonyhallett Mar 17, 2020
78b7f2e
add assertions for aria-expanded
tonyhallett Mar 17, 2020
854c0fd
Merge remote-tracking branch 'upstream/master' into allow-conditional…
tonyhallett Mar 22, 2020
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
8 changes: 7 additions & 1 deletion packages/material-ui-lab/src/TreeItem/TreeItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,13 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) {
};

React.useEffect(() => {
const childIds = React.Children.map(children, child => child.props.nodeId) || [];
const childIds = [];

React.Children.forEach(children, child => {
if (React.isValidElement(child) && child.props.nodeId) {
childIds.push(child.props.nodeId);
}
});
if (addNodeToNodeMap) {
addNodeToNodeMap(nodeId, childIds);
}
Expand Down
23 changes: 23 additions & 0 deletions packages/material-ui-lab/src/TreeItem/TreeItem.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,29 @@ describe('<TreeItem />', () => {
.to.equal('endIcon');
});

it('should allow conditional child', () => {
function TestComponent() {
const [hide, setState] = React.useState(false);

return (
<React.Fragment>
<button type="button" onClick={() => setState(true)}>
Hide
</button>
<TreeView defaultExpanded={['1']}>
<TreeItem nodeId="1" label="1" data-testid="1">
{!hide && <TreeItem nodeId="test" label="test" data-testid="2" />}
</TreeItem>
</TreeView>
</React.Fragment>
);
}
const { getByText, queryByText } = render(<TestComponent />);
expect(getByText('test')).to.not.be.null;
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use getByText. It's extremely brittle. You can leverage ByTestId since you already added testids.

Also: what is the expected aria-expanded before and after the click?

Copy link
Contributor Author

@tonyhallett tonyhallett Mar 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, as I said I mirrored the existing TreeView test. There are many getByText.

Will update

Copy link
Member

Choose a reason for hiding this comment

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

Ok, as I said I mirrored the existing TreeView test. There are many getByText.

No worries. Sometimes these slip by.

fireEvent.click(getByText('Hide'));
expect(queryByText('test')).to.be.null;
});

it('should treat an empty array equally to no children', () => {
const { getByTestId } = render(
<TreeView defaultExpanded={['1']}>
Expand Down