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

joshwooding · 2020-06-25T20:40:50Z

I have followed (at least) the PR section of the contributing guide.
Allows for more functionality in the future
Closes [TreeView] Unmount crashes React #20832 cc @eps1lon The test worked perfectly. I only realised I fixed it once the PR build failed :)
Removes limitation discussed during dogfeeding ([docs] Replace react-inspector with custom TreeView implementation #17662 (comment))

Closes #20092 (Including #20147 here to prevent merge conflicts)

Needed to update the test to cover the bug and change the fix - the previous one wouldn't have worked due to children not being set when node is collapsed. Thanks @tonyhallett. There's another change I want to make that will tidy this up even more.

mui-pr-bot · 2020-06-25T20:50:30Z

@material-ui/lab: parsed: +1.00% , gzip: +1.11%

Details of bundle changes

Generated by 🚫 dangerJS against e120da9

oliviertassinari · 2020-06-25T22:04:14Z

@joshwooding I know it's a draft. When you can get the chance, if you could like the issue(s) the change aims to solve, it would be 👌

~~@eps1lon Could something be wrong in the size comparison report? The diff seems inverted, it seems we add more code than we remove in the pull request.~~

~~https://mui-dashboard.netlify.app/size-comparison?buildId=10610&baseRef=next&baseCommit=57322a9c0ce51b3fc841216eaa7ae7d7d8da0f70&prNumber=21574~~

Edit @eps1lon: Resolve size comparison issue in https://github.com/eps1lon/mui-contributor-dashboard/pull/9

joshwooding · 2020-06-27T00:52:02Z

I've tidied up tests as part of another change.

joshwooding · 2020-06-27T15:36:49Z

packages/material-ui-lab/src/TreeItem/TreeItem.js

@@ -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';


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.

joshwooding · 2020-06-27T15:38:17Z

packages/material-ui-lab/src/TreeItem/TreeItem.test.js

@@ -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);


Consistency changes.

joshwooding · 2020-06-27T15:40:00Z

packages/material-ui-lab/src/TreeItem/TreeItem.test.js

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

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

+          expect(toggleSpy.args[0][1]).to.have.length(3);


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

joshwooding · 2020-06-27T15:40:34Z

packages/material-ui-lab/src/TreeView/TreeView.js

-
-  for (let i = 0; i < arr1.length; i += 1) {
-    if (arr1[i] !== arr2[i]) return true;
+// To replace with Object.values() once we stop IE 11 support.


I didn't forget this time 😆 I'm sure I missed something else though :P

Do we really need Object.values? Isn't it trivial to do

-Object.values(obj).forEach(value => { +Object.keys(obj).forEach(key => { + const value = obj[key] })

joshwooding · 2020-06-27T15:42:07Z

packages/material-ui-lab/src/TreeView/TreeView.js

@@ -77,6 +80,12 @@ const TreeView = React.forwardRef(function TreeView(props, ref) {
    state: 'selected',
  });

+  const getChildren = (id) =>


Bit worried about performance here due to all the looping. Something to think about I guess.

joshwooding · 2020-06-27T15:42:59Z

packages/material-ui-lab/src/TreeView/TreeView.js

-    if (nodeIndex !== -1 && nodeIndex + 1 < visibleNodes.current.length) {
-      return visibleNodes.current[nodeIndex + 1];
+    // If expanded get first child
+    if (isExpanded(id) && getChildren(id).length > 0) {


I've got some changes planned in the future which mean we won't have to check children length here.

joshwooding · 2020-06-27T15:47:34Z

packages/material-ui-lab/src/TreeView/TreeView.js

-      nodeMap.current[childId] = { ...currentChildMap, parent: id, id: childId };
-    });
-  };
+    nodeMap.current[id] = { id, index, parentId, expandable };


Registering is a lot more simple now.

joshwooding · 2020-06-27T15:48:23Z

packages/material-ui-lab/src/TreeView/TreeView.js

-        }
-      });
-      nodeMap.current = newMap;
+      cleanUpNodeMap(nodes);


unregistering nodes has also been simplified a lot.

joshwooding · 2020-06-27T15:49:25Z

packages/material-ui-lab/src/TreeView/TreeView.js

  );

  const mapFirstChar = (id, firstChar) => {
    firstCharMap.current[id] = firstChar;
  };

-  const prevChildIds = React.useRef([]);


Bye-bye, fragile code 👋

joshwooding · 2020-06-27T15:51:02Z

packages/material-ui-lab/src/TreeView/TreeView.test.js

@@ -62,9 +62,7 @@ describe('<TreeView />', () => {
      );
    });

-    // should not throw eventually or with a better error message
-    // FIXME: https://github.com/mui-org/material-ui/issues/20832
-    it('crashes when unmounting with duplicate ids', () => {


joshwooding · 2020-06-27T15:53:52Z

packages/material-ui-lab/src/TreeView/descendants.js

+import PropTypes from 'prop-types';
+
+/** Credit: https://github.com/reach/reach-ui/blob/86a046f54d53b6420e392b3fa56dd991d9d4e458/packages/descendants/README.md
+ *  Modified slightly to suit our purposes.


Modifications are to allow the parentId to be read through this context. If we wanted to use the source we could use a second context. Not sure about IE11 support though.

packages/material-ui-lab/src/TreeView/descendants.js

joshwooding · 2020-06-29T00:32:17Z

Work here is still relevant but reconsidering descendants is something to think about after the focus change: #21613 (comment)

packages/material-ui-lab/src/TreeItem/TreeItem.test.js

eps1lon · 2020-07-01T06:42:32Z

packages/material-ui-lab/src/TreeView/TreeView.js

-
-  for (let i = 0; i < arr1.length; i += 1) {
-    if (arr1[i] !== arr2[i]) return true;
+// To replace with Object.values() once we stop IE 11 support.


Do we really need Object.values? Isn't it trivial to do

-Object.values(obj).forEach(value => { +Object.keys(obj).forEach(key => { + const value = obj[key] })

packages/material-ui-lab/src/TreeView/TreeView.js

packages/material-ui-lab/src/TreeView/descendants.js

packages/material-ui-lab/src/TreeView/TreeView.js

joshwooding added new feature New feature or request component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! labels Jun 25, 2020

joshwooding mentioned this pull request Jun 26, 2020

[TreeView] Fix expandAllSiblings expands end nodes #20147

Closed

1 task

joshwooding changed the title ~~[TreeView] Improve node registration~~ [TreeView] Improve node registration and fix other issues Jun 26, 2020

joshwooding force-pushed the descendants-tree-view branch 2 times, most recently from 48c5de8 to cf75994 Compare June 27, 2020 00:56

joshwooding requested review from eps1lon and oliviertassinari June 27, 2020 00:57

joshwooding marked this pull request as ready for review June 27, 2020 00:57

joshwooding commented Jun 27, 2020

View reviewed changes

joshwooding force-pushed the descendants-tree-view branch from cf75994 to 154c8f1 Compare June 27, 2020 16:00

joshwooding mentioned this pull request Jun 27, 2020

[TreeView] Change focus management to aria-descendant #21592

Closed

1 task

github-actions bot mentioned this pull request Jun 27, 2020

Update snapshots for pr/21574 eps1lon/mui-scripts-incubator#459

Closed

oliviertassinari reviewed Jun 28, 2020

View reviewed changes

joshwooding requested a review from oliviertassinari June 28, 2020 22:16

oliviertassinari approved these changes Jun 28, 2020

View reviewed changes

packages/material-ui-lab/src/TreeView/descendants.js Show resolved Hide resolved

joshwooding added the bug 🐛 Something doesn't work label Jun 28, 2020

This comment has been minimized.

Sign in to view

eps1lon reviewed Jul 1, 2020

View reviewed changes

packages/material-ui-lab/src/TreeItem/TreeItem.test.js Show resolved Hide resolved

eps1lon requested changes Jul 1, 2020

View reviewed changes

eps1lon mentioned this pull request Jul 1, 2020

fix: merge base is the previous size mui/mui-public#9

Merged

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 2, 2020

eps1lon reviewed Jul 2, 2020

View reviewed changes

packages/material-ui-lab/src/TreeView/TreeView.js Outdated Show resolved Hide resolved

eps1lon reviewed Jul 2, 2020

View reviewed changes

packages/material-ui-lab/src/TreeView/TreeView.js Outdated Show resolved Hide resolved

eps1lon reviewed Jul 2, 2020

View reviewed changes

packages/material-ui-lab/src/TreeView/TreeView.js Show resolved Hide resolved

eps1lon reviewed Jul 2, 2020

View reviewed changes

packages/material-ui-lab/src/TreeView/TreeView.js Outdated Show resolved Hide resolved

joshwooding added 7 commits July 3, 2020 00:32

First step

8f855d0

Remove visibleNodes and use descendants.

911e2e6

Re-purpose bug test

3edb4e4

Use mui#20147 and modify slightly

8f3bccd

olivier review

753fece

sebastian's review

fceb38d

Re-word

3645d20

joshwooding force-pushed the descendants-tree-view branch from 2dbbc84 to 3645d20 Compare July 2, 2020 23:39

More readable?

e120da9

joshwooding requested a review from eps1lon July 3, 2020 00:39

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jul 3, 2020

eps1lon approved these changes Jul 3, 2020

View reviewed changes

oliviertassinari approved these changes Jul 3, 2020

View reviewed changes

joshwooding merged commit a2c0ff5 into mui:next Jul 3, 2020

joshwooding mentioned this pull request Jul 23, 2020

TreeView multiselect shift click does not work when items wrapped in fragment #21848

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

joshwooding commented Jun 25, 2020 •

edited

mui-pr-bot commented Jun 25, 2020 •

edited

oliviertassinari commented Jun 25, 2020 •

edited by eps1lon

joshwooding commented Jun 27, 2020

joshwooding Jun 27, 2020 •

edited

joshwooding Jun 27, 2020

joshwooding Jun 27, 2020

joshwooding Jun 27, 2020

eps1lon Jul 1, 2020

joshwooding Jun 27, 2020

joshwooding Jun 27, 2020

joshwooding Jun 27, 2020

joshwooding Jun 27, 2020

joshwooding Jun 27, 2020 •

edited

joshwooding Jun 27, 2020

joshwooding Jun 27, 2020 •

edited

joshwooding commented Jun 29, 2020

This comment has been minimized.

eps1lon Jul 1, 2020

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

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

		expect(toggleSpy.args[0][1]).to.have.length(3);

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

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

Conversation

joshwooding commented Jun 25, 2020 • edited

mui-pr-bot commented Jun 25, 2020 • edited

oliviertassinari commented Jun 25, 2020 • edited by eps1lon

joshwooding commented Jun 27, 2020

joshwooding Jun 27, 2020 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshwooding Jun 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshwooding Jun 27, 2020 • edited

Choose a reason for hiding this comment

joshwooding commented Jun 29, 2020

This comment has been minimized.

Choose a reason for hiding this comment

joshwooding commented Jun 25, 2020 •

edited

mui-pr-bot commented Jun 25, 2020 •

edited

oliviertassinari commented Jun 25, 2020 •

edited by eps1lon

joshwooding Jun 27, 2020 •

edited

joshwooding Jun 27, 2020 •

edited

joshwooding Jun 27, 2020 •

edited