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

Jump to bottom

[TreeView] Improve node registration and fix other issues #21574

Merged
merged 8 commits into from Jul 3, 2020
Merged

[TreeView] Improve node registration and fix other issues #21574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
8f855d0
First step
joshwooding Jun 21, 2020
911e2e6
Remove visibleNodes and use descendants.
joshwooding Jun 25, 2020
3edb4e4
Re-purpose bug test
joshwooding Jun 25, 2020
8f3bccd
Use #20147 and modify slightly
joshwooding Jun 26, 2020
753fece
olivier review
joshwooding Jun 28, 2020
fceb38d
sebastian's review
joshwooding Jul 1, 2020
3645d20
Re-word
joshwooding Jul 2, 2020
e120da9
More readable?
joshwooding Jul 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
57 changes: 32 additions & 25 deletions packages/material-ui-lab/src/TreeItem/TreeItem.js
View file
Open in desktop
Expand Up @@ -7,6 +7,7 @@ import Collapse from '@material-ui/core/Collapse';
import { fade, withStyles, useTheme } from '@material-ui/core/styles';
import { useForkRef } from '@material-ui/core/utils';
import TreeViewContext from '../TreeView/TreeViewContext';
import { DescendantProvider, useDescendant, useDescendantsInit } from '../TreeView/descendants';
Copy link
Member Author

Choose a reason for hiding this comment

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

Using descendants allows us to get tree items in the correct order without reading children' props meaning we aren't so fragile and allow wrapped TreeItems.


export const styles = (theme) => ({
/* Styles applied to the root element. */
Expand Down Expand Up @@ -135,14 +136,19 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) {
multiSelect,
getParent,
mapFirstChar,
addNodeToNodeMap,
removeNodeFromNodeMap,
registerNode,
unregisterNode,
} = React.useContext(TreeViewContext);

const nodeRef = React.useRef(null);
const contentRef = React.useRef(null);
const handleRef = useForkRef(nodeRef, ref);

const { index, parentId } = useDescendant({
element: nodeRef.current,
id: nodeId,
});

let icon = iconProp;

const expandable = Boolean(Array.isArray(children) ? children.length : children);
Expand Down Expand Up @@ -337,25 +343,22 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) {
};

React.useEffect(() => {
if (addNodeToNodeMap) {
const childIds = [];
React.Children.forEach(children, (child) => {
if (React.isValidElement(child) && child.props.nodeId) {
childIds.push(child.props.nodeId);
}
// On the first render a node's index will be -1. We want to wait for the real index.
if (registerNode && unregisterNode && index !== -1) {
registerNode({
id: nodeId,
index,
parentId,
expandable,
});
addNodeToNodeMap(nodeId, childIds);
}
}, [children, nodeId, addNodeToNodeMap]);

React.useEffect(() => {
if (removeNodeFromNodeMap) {
return () => {
removeNodeFromNodeMap(nodeId);
unregisterNode(nodeId);
};
}

return undefined;
}, [nodeId, removeNodeFromNodeMap]);
}, [registerNode, unregisterNode, parentId, index, nodeId, expandable]);

React.useEffect(() => {
if (mapFirstChar && label) {
Expand All @@ -382,6 +385,8 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) {
ariaSelected = true;
}

const [descendants, setDescendants] = useDescendantsInit();

return (
<li
className={clsx(classes.root, className)}
Expand Down Expand Up @@ -412,16 +417,18 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) {
</Typography>
</div>
{children && (
<TransitionComponent
unmountOnExit
className={classes.group}
in={expanded}
component="ul"
role="group"
{...TransitionProps}
>
{children}
</TransitionComponent>
<DescendantProvider items={descendants} set={setDescendants} id={nodeId}>
<TransitionComponent
unmountOnExit
className={classes.group}
in={expanded}
component="ul"
role="group"
{...TransitionProps}
>
{children}
</TransitionComponent>
</DescendantProvider>
)}
</li>
);
Expand Down
48 changes: 25 additions & 23 deletions packages/material-ui-lab/src/TreeItem/TreeItem.test.js
View file
Open in desktop
Expand Up @@ -333,13 +333,10 @@ describe('<TreeItem />', () => {

it('should work with programmatic focus', () => {
const { getByTestId } = render(
<React.Fragment>
<div data-testid="start" tabIndex={0} />
eps1lon marked this conversation as resolved.
Show resolved Hide resolved
<TreeView>
<TreeItem nodeId="1" label="one" data-testid="one" />
<TreeItem nodeId="2" label="two" data-testid="two" />
</TreeView>
</React.Fragment>,
<TreeView>
<TreeItem nodeId="1" label="one" data-testid="one" />
<TreeItem nodeId="2" label="two" data-testid="two" />
</TreeView>,
);

expect(getByTestId('one')).to.have.attribute('tabindex', '0');
Expand Down Expand Up @@ -840,8 +837,9 @@ describe('<TreeItem />', () => {

describe('asterisk key interaction', () => {
it('expands all siblings that are at the same level as the current node', () => {
const toggleSpy = spy();
const { getByTestId } = render(
<TreeView>
<TreeView onNodeToggle={toggleSpy}>
<TreeItem nodeId="one" label="one" data-testid="one">
<TreeItem nodeId="two" label="two" data-testid="two" />
</TreeItem>
Expand All @@ -853,6 +851,7 @@ describe('<TreeItem />', () => {
<TreeItem nodeId="seven" label="seven" data-testid="seven" />
</TreeItem>
</TreeItem>
<TreeItem nodeId="eight" label="eight" data-testid="eight" />
</TreeView>,
);

Expand All @@ -864,10 +863,13 @@ describe('<TreeItem />', () => {

fireEvent.keyDown(getByTestId('one'), { key: '*' });

expect(toggleSpy.args[0][1]).to.have.length(3);
Copy link
Member Author

Choose a reason for hiding this comment

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

We need to check the change handler here because eight will never have aria-expanded set due to a check when applying that attribute.


expect(getByTestId('one')).to.have.attribute('aria-expanded', 'true');
expect(getByTestId('three')).to.have.attribute('aria-expanded', 'true');
expect(getByTestId('five')).to.have.attribute('aria-expanded', 'true');
expect(getByTestId('six')).to.have.attribute('aria-expanded', 'false');
expect(getByTestId('eight')).not.to.have.attribute('aria-expanded');
});
});
});
Expand Down Expand Up @@ -990,27 +992,27 @@ describe('<TreeItem />', () => {
fireEvent.keyDown(getByTestId('three'), { key: 'ArrowDown', shiftKey: true });

expect(getByTestId('four')).toHaveFocus();
expect(container.querySelectorAll('[aria-selected=true]').length).to.equal(2);
expect(container.querySelectorAll('[aria-selected=true]')).to.have.length(2);
Copy link
Member Author

Choose a reason for hiding this comment

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

Consistency changes.


fireEvent.keyDown(getByTestId('four'), { key: 'ArrowDown', shiftKey: true });

expect(getByTestId('three')).to.have.attribute('aria-selected', 'true');
expect(getByTestId('four')).to.have.attribute('aria-selected', 'true');
expect(getByTestId('five')).to.have.attribute('aria-selected', 'true');
expect(container.querySelectorAll('[aria-selected=true]').length).to.equal(3);
expect(container.querySelectorAll('[aria-selected=true]')).to.have.length(3);

fireEvent.keyDown(getByTestId('five'), { key: 'ArrowUp', shiftKey: true });

expect(getByTestId('four')).toHaveFocus();
expect(container.querySelectorAll('[aria-selected=true]').length).to.equal(2);
expect(container.querySelectorAll('[aria-selected=true]')).to.have.length(2);

fireEvent.keyDown(getByTestId('four'), { key: 'ArrowUp', shiftKey: true });

expect(container.querySelectorAll('[aria-selected=true]').length).to.equal(1);
expect(container.querySelectorAll('[aria-selected=true]')).to.have.length(1);

fireEvent.keyDown(getByTestId('three'), { key: 'ArrowUp', shiftKey: true });

expect(container.querySelectorAll('[aria-selected=true]').length).to.equal(2);
expect(container.querySelectorAll('[aria-selected=true]')).to.have.length(2);

fireEvent.keyDown(getByTestId('two'), { key: 'ArrowUp', shiftKey: true });

Expand All @@ -1019,7 +1021,7 @@ describe('<TreeItem />', () => {
expect(getByTestId('three')).to.have.attribute('aria-selected', 'true');
expect(getByTestId('four')).to.have.attribute('aria-selected', 'false');
expect(getByTestId('five')).to.have.attribute('aria-selected', 'false');
expect(container.querySelectorAll('[aria-selected=true]').length).to.equal(3);
expect(container.querySelectorAll('[aria-selected=true]')).to.have.length(3);
});

specify('keyboard arrow does not select when selectionDisabled', () => {
Expand All @@ -1037,11 +1039,11 @@ describe('<TreeItem />', () => {
fireEvent.keyDown(getByTestId('three'), { key: 'ArrowDown', shiftKey: true });

expect(getByTestId('four')).toHaveFocus();
expect(container.querySelectorAll('[aria-selected=true]').length).to.equal(0);
expect(container.querySelectorAll('[aria-selected=true]')).to.have.length(0);

fireEvent.keyDown(getByTestId('four'), { key: 'ArrowUp', shiftKey: true });

expect(container.querySelectorAll('[aria-selected=true]').length).to.equal(0);
expect(container.querySelectorAll('[aria-selected=true]')).to.have.length(0);
});

specify('keyboard arrow merge', () => {
Expand All @@ -1067,12 +1069,12 @@ describe('<TreeItem />', () => {
fireEvent.keyDown(getByTestId('four'), { key: 'ArrowUp', shiftKey: true });
fireEvent.keyDown(getByTestId('three'), { key: 'ArrowUp', shiftKey: true });

expect(container.querySelectorAll('[aria-selected=true]').length).to.equal(5);
expect(container.querySelectorAll('[aria-selected=true]')).to.have.length(5);

fireEvent.keyDown(getByTestId('two'), { key: 'ArrowDown', shiftKey: true });
fireEvent.keyDown(getByTestId('three'), { key: 'ArrowDown', shiftKey: true });

expect(container.querySelectorAll('[aria-selected=true]').length).to.equal(3);
expect(container.querySelectorAll('[aria-selected=true]')).to.have.length(3);
});

specify('keyboard space', () => {
Expand Down Expand Up @@ -1179,11 +1181,11 @@ describe('<TreeItem />', () => {
getByTestId('five').focus();
fireEvent.keyDown(getByTestId('five'), { key: 'End', shiftKey: true, ctrlKey: true });

expect(container.querySelectorAll('[aria-selected=true]').length).to.equal(0);
expect(container.querySelectorAll('[aria-selected=true]')).to.have.length(0);

fireEvent.keyDown(getByTestId('nine'), { key: 'Home', shiftKey: true, ctrlKey: true });

expect(container.querySelectorAll('[aria-selected=true]').length).to.equal(0);
expect(container.querySelectorAll('[aria-selected=true]')).to.have.length(0);
});

specify('mouse', () => {
Expand Down Expand Up @@ -1237,7 +1239,7 @@ describe('<TreeItem />', () => {

fireEvent.click(getByText('five'));
fireEvent.click(getByText('nine'), { shiftKey: true });
expect(container.querySelectorAll('[aria-selected=true]').length).to.equal(0);
expect(container.querySelectorAll('[aria-selected=true]')).to.have.length(0);
});
});

Expand Down Expand Up @@ -1318,7 +1320,7 @@ describe('<TreeItem />', () => {
getByTestId('one').focus();
fireEvent.keyDown(getByTestId('one'), { key: 'a', ctrlKey: true });

expect(container.querySelectorAll('[aria-selected=true]').length).to.equal(5);
expect(container.querySelectorAll('[aria-selected=true]')).to.have.length(5);
});

specify('ctrl + a does not select all when disableSelection', () => {
Expand All @@ -1335,7 +1337,7 @@ describe('<TreeItem />', () => {
getByTestId('one').focus();
fireEvent.keyDown(getByTestId('one'), { key: 'a', ctrlKey: true });

expect(container.querySelectorAll('[aria-selected=true]').length).to.equal(0);
expect(container.querySelectorAll('[aria-selected=true]')).to.have.length(0);
});
});
});
Expand Down