From 8f855d0c184646697f7f876ddff85f84bd6c443c Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sun, 21 Jun 2020 16:21:22 +0100 Subject: [PATCH 1/8] First step --- .../material-ui-lab/src/TreeItem/TreeItem.js | 21 +- .../material-ui-lab/src/TreeView/TreeView.js | 31 ++- .../src/TreeView/descendants.js | 224 ++++++++++++++++++ 3 files changed, 255 insertions(+), 21 deletions(-) create mode 100644 packages/material-ui-lab/src/TreeView/descendants.js diff --git a/packages/material-ui-lab/src/TreeItem/TreeItem.js b/packages/material-ui-lab/src/TreeItem/TreeItem.js index a79cd986b94f2f..b81b39870931ee 100644 --- a/packages/material-ui-lab/src/TreeItem/TreeItem.js +++ b/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 { useDescendant } from '../TreeView/descendants'; export const styles = (theme) => ({ /* Styles applied to the root element. */ @@ -135,14 +136,18 @@ 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 = useDescendant({ + element: nodeRef.current, + }); + let icon = iconProp; const expandable = Boolean(Array.isArray(children) ? children.length : children); @@ -337,25 +342,25 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) { }; React.useEffect(() => { - if (addNodeToNodeMap) { + if (registerNode) { const childIds = []; React.Children.forEach(children, (child) => { if (React.isValidElement(child) && child.props.nodeId) { childIds.push(child.props.nodeId); } }); - addNodeToNodeMap(nodeId, childIds); + registerNode(nodeId, childIds); } - }, [children, nodeId, addNodeToNodeMap]); + }, [children, nodeId, registerNode]); React.useEffect(() => { - if (removeNodeFromNodeMap) { + if (unregisterNode) { return () => { - removeNodeFromNodeMap(nodeId); + unregisterNode(nodeId); }; } return undefined; - }, [nodeId, removeNodeFromNodeMap]); + }, [nodeId, unregisterNode]); React.useEffect(() => { if (mapFirstChar && label) { diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js index bd64ce8ae6fcee..572a45acf70f55 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.js @@ -4,6 +4,7 @@ import PropTypes from 'prop-types'; import { withStyles } from '@material-ui/core/styles'; import { useControlled } from '@material-ui/core/utils'; import TreeViewContext from './TreeViewContext'; +import { DescendantProvider, useDescendantsInit } from './descendants'; export const styles = { /* Styles applied to the root element. */ @@ -395,7 +396,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { * Mapping Helpers */ - const addNodeToNodeMap = (id, childrenIds) => { + const registerNode = (id, childrenIds) => { const currentMap = nodeMap.current[id]; nodeMap.current[id] = { ...currentMap, children: childrenIds, id }; @@ -430,7 +431,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { firstCharMap.current = newMap; }, []); - const removeNodeFromNodeMap = React.useCallback( + const unregisterNode = React.useCallback( (id) => { const nodes = getNodesToRemove(id); cleanUpFirstCharMap(nodes); @@ -513,6 +514,8 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { return false; }; + const [descendants, setDescendants] = useDescendantsInit(); + return ( - + + + ); }); diff --git a/packages/material-ui-lab/src/TreeView/descendants.js b/packages/material-ui-lab/src/TreeView/descendants.js new file mode 100644 index 00000000000000..76d90c413be508 --- /dev/null +++ b/packages/material-ui-lab/src/TreeView/descendants.js @@ -0,0 +1,224 @@ +import * as React from 'react'; +import PropTypes from 'prop-types'; + +// To replace with .findIndex() once we stop IE 11 support. +function findIndex(array, comp) { + for (let i = 0; i < array.length; i += 1) { + if (comp(array[i])) { + return i; + } + } + + return -1; +} + +const useEnhancedEffect = + typeof window !== 'undefined' && process.env.NODE_ENV !== 'test' + ? React.useLayoutEffect + : React.useEffect; + +const DescendantContext = React.createContext({}); + +if (process.env.NODE_ENV !== 'production') { + DescendantContext.displayName = 'DescendantContext'; +} + +function usePrevious(value) { + const ref = React.useRef(null); + React.useEffect(() => { + ref.current = value; + }, [value]); + return ref.current; +} + +const noop = () => {}; + +/** + * This hook registers our descendant by passing it into an array. We can then + * search that array by to find its index when registering it in the component. + * We use this for focus management, keyboard navigation, and typeahead + * functionality for some components. + * + * The hook accepts the element node + * + * Our main goals with this are: + * 1) maximum composability, + * 2) minimal API friction + * 3) SSR compatibility* + * 4) concurrent safe + * 5) index always up-to-date with the tree despite changes + * 6) works with memoization of any component in the tree (hopefully) + * + * * As for SSR, the good news is that we don't actually need the index on the + * server for most use-cases, as we are only using it to determine the order of + * composed descendants for keyboard navigation. + */ +export function useDescendant(descendant) { + const [, forceUpdate] = React.useState(); + const { + registerDescendant = noop, + unregisterDescendant = noop, + descendants = [], + } = React.useContext(DescendantContext); + + // This will initially return -1 because we haven't registered the descendant + // on the first render. After we register, this will then return the correct + // index on the following render and we will re-register descendants + // so that everything is up-to-date before the user interacts with a + // collection. + const index = findIndex(descendants, (item) => item.element === descendant.element); + + const previousDescendants = usePrevious(descendants); + + // We also need to re-register descendants any time ANY of the other + // descendants have changed. My brain was melting when I wrote this and it + // feels a little off, but checking in render and using the result in the + // effect's dependency array works well enough. + const someDescendantsHaveChanged = descendants.some((d, i) => { + return d.element !== previousDescendants?.[i]?.element; + }); + + // Prevent any flashing + useEnhancedEffect(() => { + if (!descendant.element) forceUpdate({}); + registerDescendant({ + ...descendant, + index, + }); + return () => unregisterDescendant(descendant.element); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [ + registerDescendant, + unregisterDescendant, + index, + someDescendantsHaveChanged, + // eslint-disable-next-line react-hooks/exhaustive-deps + ...Object.values(descendant), + ]); + + return index; +} + +export function useDescendantsInit() { + return React.useState([]); +} + +export function useDescendants() { + return React.useContext(DescendantContext).descendants; +} + +export function DescendantProvider(props) { + const { children, items, set } = props; + + const registerDescendant = React.useCallback( + ({ element, index: explicitIndex, ...rest }) => { + if (!element) { + return; + } + + set((oldItems) => { + let newItems; + if (explicitIndex != null) { + newItems = [ + ...oldItems, + { + ...rest, + element, + index: explicitIndex, + }, + ]; + } else if (oldItems.length === 0) { + // If there are no items, register at index 0 and bail. + newItems = [ + ...oldItems, + { + ...rest, + element, + index: 0, + }, + ]; + } else if (oldItems.find((item) => item.element === element)) { + // If the element is already registered, just use the same array + newItems = oldItems; + } else { + // When registering a descendant, we need to make sure we insert in + // into the array in the same order that it appears in the DOM. So as + // new descendants are added or maybe some are removed, we always know + // that the array is up-to-date and correct. + // + // So here we look at our registered descendants and see if the new + // element we are adding appears earlier than an existing descendant's + // DOM node via `node.compareDocumentPosition`. If it does, we insert + // the new element at this index. Because `registerDescendant` will be + // called in an effect every time the descendants state value changes, + // we should be sure that this index is accurate when descendent + // elements come or go from our component. + const index = findIndex(oldItems, (item) => { + if (!item.element || !element) { + return false; + } + // Does this element's DOM node appear before another item in the + // array in our DOM tree? If so, return true to grab the index at + // this point in the array so we know where to insert the new + // element. + return Boolean( + // eslint-disable-next-line no-bitwise + item.element.compareDocumentPosition(element) & Node.DOCUMENT_POSITION_PRECEDING, + ); + }); + + const newItem = { + ...rest, + element, + index, + }; + + // If an index is not found we will push the element to the end. + if (index === -1) { + newItems = [...oldItems, newItem]; + } else { + newItems = [...oldItems.slice(0, index), newItem, ...oldItems.slice(index)]; + } + } + return newItems.map((item, index) => ({ ...item, index })); + }); + }, + // set is a state setter initialized by the useDescendants hook. + // We can safely ignore the lint warning here because it will not change + // between renders. + // eslint-disable-next-line react-hooks/exhaustive-deps + [], + ); + + const unregisterDescendant = React.useCallback( + (element) => { + if (!element) { + return; + } + + set((oldItems) => oldItems.filter((item) => element !== item.element)); + }, + // set is a state setter initialized by the useDescendants hook. + // We can safely ignore the lint warning here because it will not change + // between renders. + // eslint-disable-next-line react-hooks/exhaustive-deps + [], + ); + + const value = React.useMemo( + () => ({ + descendants: items, + registerDescendant, + unregisterDescendant, + }), + [items, registerDescendant, unregisterDescendant], + ); + + return {children}; +} + +DescendantProvider.propTypes = { + children: PropTypes.node, + items: PropTypes.array, + set: PropTypes.func, +}; From 911e2e6adc6b9b3a6ded435e3701e3bea9dc36e7 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Thu, 25 Jun 2020 17:15:37 +0100 Subject: [PATCH 2/8] Remove visibleNodes and use descendants. --- .../material-ui-lab/src/TreeItem/TreeItem.js | 48 ++-- .../src/TreeItem/TreeItem.test.js | 13 +- .../material-ui-lab/src/TreeView/TreeView.js | 266 +++++++++++------- .../src/TreeView/descendants.js | 29 +- 4 files changed, 197 insertions(+), 159 deletions(-) diff --git a/packages/material-ui-lab/src/TreeItem/TreeItem.js b/packages/material-ui-lab/src/TreeItem/TreeItem.js index b81b39870931ee..6fe6750a78f872 100644 --- a/packages/material-ui-lab/src/TreeItem/TreeItem.js +++ b/packages/material-ui-lab/src/TreeItem/TreeItem.js @@ -7,7 +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 { useDescendant } from '../TreeView/descendants'; +import { DescendantProvider, useDescendant, useDescendantsInit } from '../TreeView/descendants'; export const styles = (theme) => ({ /* Styles applied to the root element. */ @@ -144,8 +144,9 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) { const contentRef = React.useRef(null); const handleRef = useForkRef(nodeRef, ref); - const index = useDescendant({ + const { index, parentId } = useDescendant({ element: nodeRef.current, + id: nodeId, }); let icon = iconProp; @@ -342,25 +343,20 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) { }; React.useEffect(() => { - if (registerNode) { - const childIds = []; - React.Children.forEach(children, (child) => { - if (React.isValidElement(child) && child.props.nodeId) { - childIds.push(child.props.nodeId); - } + if (registerNode && unregisterNode && index !== -1) { + registerNode({ + id: nodeId, + index, + parentId, }); - registerNode(nodeId, childIds); - } - }, [children, nodeId, registerNode]); - React.useEffect(() => { - if (unregisterNode) { return () => { unregisterNode(nodeId); }; } + return undefined; - }, [nodeId, unregisterNode]); + }, [registerNode, unregisterNode, parentId, index, nodeId]); React.useEffect(() => { if (mapFirstChar && label) { @@ -387,6 +383,8 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) { ariaSelected = true; } + const [descendants, setDescendants] = useDescendantsInit(); + return (
  • {children && ( - - {children} - + + + {children} + + )}
    • ); diff --git a/packages/material-ui-lab/src/TreeItem/TreeItem.test.js b/packages/material-ui-lab/src/TreeItem/TreeItem.test.js index 0a67a33529abec..04b2a712c666cd 100644 --- a/packages/material-ui-lab/src/TreeItem/TreeItem.test.js +++ b/packages/material-ui-lab/src/TreeItem/TreeItem.test.js @@ -333,13 +333,10 @@ describe('', () => { it('should work with programmatic focus', () => { const { getByTestId } = render( - -
    - - - - - , + + + + , ); expect(getByTestId('one')).to.have.attribute('tabindex', '0'); @@ -853,6 +850,7 @@ describe('', () => { + , ); @@ -868,6 +866,7 @@ describe('', () => { 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'); }); }); }); diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js index 572a45acf70f55..99773af868b86d 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.js @@ -15,14 +15,16 @@ export const styles = { }, }; -function arrayDiff(arr1, arr2) { - if (arr1.length !== arr2.length) return true; - - 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. +function objectValues(obj) { + const res = []; + // eslint-disable-next-line no-restricted-syntax + for (const i in obj) { + if (obj.hasOwnProperty(i)) { + res.push(obj[i]); + } } - - return false; + return res; } const findNextFirstChar = (firstChars, startIndex, char) => { @@ -58,11 +60,11 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { } = props; const [tabbable, setTabbable] = React.useState(null); const [focusedNodeId, setFocusedNodeId] = React.useState(null); + const [descendants, setDescendants] = useDescendantsInit(); const nodeMap = React.useRef({}); const firstCharMap = React.useRef({}); - const visibleNodes = React.useRef([]); const [expanded, setExpandedState] = useControlled({ controlled: expandedProp, @@ -78,6 +80,12 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { state: 'selected', }); + const getChildren = (id) => + objectValues(nodeMap.current) + .filter((node) => node.parentId === id) + .sort((a, b) => a.index - b.index) + .map((child) => child.id); + /* * Status Helpers */ @@ -99,31 +107,126 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { */ const getNextNode = (id) => { - const nodeIndex = visibleNodes.current.indexOf(id); - 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) { + return getChildren(id)[0]; + } + + // Try to get next sibling + const node = nodeMap.current[id]; + const siblings = getChildren(node.parentId); + + const nextSibling = siblings[siblings.indexOf(id) + 1]; + + if (nextSibling) { + return nextSibling; + } + + // try to get parent's next sibling + const parent = nodeMap.current[node.parentId]; + if (parent) { + const parentSiblings = getChildren(parent.parentId); + return parentSiblings[parentSiblings.indexOf(parent.id) + 1]; } return null; }; const getPreviousNode = (id) => { - const nodeIndex = visibleNodes.current.indexOf(id); - if (nodeIndex !== -1 && nodeIndex - 1 >= 0) { - return visibleNodes.current[nodeIndex - 1]; + const node = nodeMap.current[id]; + const siblings = getChildren(node.parentId); + const nodeIndex = siblings.indexOf(id); + + if (nodeIndex === 0) { + return node.parentId; } - return null; + + // start with previous node + let currentNode = siblings[nodeIndex - 1]; + while (isExpanded(currentNode) && getChildren(currentNode).length > 0) { + currentNode = getChildren(currentNode).pop(); + } + + return currentNode; + }; + + const getLastNode = () => { + let lastNode = getChildren(null).pop(); + + while (isExpanded(lastNode)) { + lastNode = getChildren(lastNode).pop(); + } + return lastNode; }; + const getFirstNode = () => getChildren(null)[0]; + const getParent = (id) => nodeMap.current[id].parentId; + + const getOrder = (nodeAId, nodeBId) => { + if (nodeAId === nodeBId) { + return [nodeAId, nodeBId]; + } + + const nodeA = nodeMap.current[nodeAId]; + const nodeB = nodeMap.current[nodeBId]; + + if (nodeA.parentId === nodeB.id || nodeB.parentId === nodeA.id) { + return nodeB.parentId === nodeA.id ? [nodeA.id, nodeB.id] : [nodeB.id, nodeA.id]; + } + + const aFamily = [nodeA.id]; + const bFamily = [nodeB.id]; + + let aAncestor = nodeA.parentId; + let bAncestor = nodeB.parentId; + + let aAncestorIsCommon = bFamily.indexOf(aAncestor) !== -1; + let bAncestorIsCommon = aFamily.indexOf(bAncestor) !== -1; + + let continueA = true; + let continueB = true; + + while (!bAncestorIsCommon && !aAncestorIsCommon) { + if (continueA) { + aFamily.push(aAncestor); + aAncestorIsCommon = bFamily.indexOf(aAncestor) !== -1; + continueA = aAncestor !== null; + if (!aAncestorIsCommon && continueA) { + aAncestor = nodeMap.current[aAncestor].parentId; + } + } + + if (continueB && !aAncestorIsCommon) { + bFamily.push(bAncestor); + bAncestorIsCommon = aFamily.indexOf(bAncestor) !== -1; + continueB = bAncestor !== null; + if (!bAncestorIsCommon && continueB) { + bAncestor = nodeMap.current[bAncestor].parentId; + } + } + } + + const commonAncestor = aAncestorIsCommon ? aAncestor : bAncestor; + const ancestorFamily = getChildren(commonAncestor); - const getLastNode = () => visibleNodes.current[visibleNodes.current.length - 1]; - const getFirstNode = () => visibleNodes.current[0]; - const getParent = (id) => nodeMap.current[id].parent; + const aSide = aFamily[aFamily.indexOf(commonAncestor) - 1]; + const bSide = bFamily[bFamily.indexOf(commonAncestor) - 1]; + + return ancestorFamily.indexOf(aSide) < ancestorFamily.indexOf(bSide) + ? [nodeAId, nodeBId] + : [nodeBId, nodeAId]; + }; const getNodesInRange = (a, b) => { - const aIndex = visibleNodes.current.indexOf(a); - const bIndex = visibleNodes.current.indexOf(b); - const start = Math.min(aIndex, bIndex); - const end = Math.max(aIndex, bIndex); - return visibleNodes.current.slice(start, end + 1); + const [start, end] = getOrder(a, b); + const nodes = [start]; + + let current = start; + + while (current !== end) { + current = getNextNode(current); + nodes.push(current); + } + + return nodes; }; /* @@ -153,7 +256,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { Object.keys(firstCharMap.current).forEach((nodeId) => { const firstChar = firstCharMap.current[nodeId]; const map = nodeMap.current[nodeId]; - const visible = map.parent ? isExpanded(map.parent) : true; + const visible = map.parentId ? isExpanded(map.parentId) : true; if (visible) { firstCharIds.push(nodeId); @@ -187,11 +290,12 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { const toggleExpansion = (event, value = focusedNodeId) => { let newExpanded; + if (expanded.indexOf(value) !== -1) { newExpanded = expanded.filter((id) => id !== value); setTabbable((oldTabbable) => { const map = nodeMap.current[oldTabbable]; - if (oldTabbable && (map && map.parent ? map.parent.id : null) === value) { + if (oldTabbable && map && map.parentId === value) { return value; } return oldTabbable; @@ -209,15 +313,10 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { const expandAllSiblings = (event, id) => { const map = nodeMap.current[id]; - const parent = nodeMap.current[map.parent]; + const siblings = getChildren(map.parentId); + + const diff = siblings.filter((child) => !isExpanded(child)); - let diff; - if (parent) { - diff = parent.children.filter((child) => !isExpanded(child)); - } else { - const topLevelNodes = nodeMap.current[-1].children; - diff = topLevelNodes.filter((node) => !isExpanded(node)); - } const newExpanded = expanded.concat(diff); if (diff.length > 0) { @@ -395,25 +494,21 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { /* * Mapping Helpers */ + const registerNode = React.useCallback((node) => { + const { id, index, parentId } = node; - const registerNode = (id, childrenIds) => { - const currentMap = nodeMap.current[id]; - nodeMap.current[id] = { ...currentMap, children: childrenIds, id }; - - childrenIds.forEach((childId) => { - const currentChildMap = nodeMap.current[childId]; - nodeMap.current[childId] = { ...currentChildMap, parent: id, id: childId }; - }); - }; + nodeMap.current[id] = { id, index, parentId }; + }, []); const getNodesToRemove = React.useCallback((id) => { const map = nodeMap.current[id]; const nodes = []; if (map) { nodes.push(id); - if (map.children) { - nodes.concat(map.children); - map.children.forEach((node) => { + const childNodes = getChildren(id); + if (childNodes.length > 0) { + nodes.concat(childNodes); + childNodes.forEach((node) => { nodes.concat(getNodesToRemove(node)); }); } @@ -424,98 +519,53 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { const cleanUpFirstCharMap = React.useCallback((nodes) => { const newMap = { ...firstCharMap.current }; nodes.forEach((node) => { - if (newMap[node]) { - delete newMap[node]; - } + delete newMap[node]; }); firstCharMap.current = newMap; }, []); + const cleanUpNodeMap = React.useCallback((nodes) => { + const newMap = { ...nodeMap.current }; + nodes.forEach((node) => { + delete newMap[node]; + }); + nodeMap.current = newMap; + }, []); + const unregisterNode = React.useCallback( (id) => { const nodes = getNodesToRemove(id); cleanUpFirstCharMap(nodes); - const newMap = { ...nodeMap.current }; - - nodes.forEach((node) => { - const map = newMap[node]; - if (map) { - if (map.parent) { - const parentMap = newMap[map.parent]; - if (parentMap && parentMap.children) { - const parentChildren = parentMap.children.filter((c) => c !== node); - newMap[map.parent] = { ...parentMap, children: parentChildren }; - } - } - - delete newMap[node]; - } - }); - nodeMap.current = newMap; + cleanUpNodeMap(nodes); setFocusedNodeId((oldFocusedNodeId) => { - if (oldFocusedNodeId === id) { + if (nodeMap.current[oldFocusedNodeId] === undefined) { return null; } return oldFocusedNodeId; }); }, - [getNodesToRemove, cleanUpFirstCharMap], + [getNodesToRemove, cleanUpFirstCharMap, cleanUpNodeMap], ); const mapFirstChar = (id, firstChar) => { firstCharMap.current[id] = firstChar; }; - const prevChildIds = React.useRef([]); - const [childrenCalculated, setChildrenCalculated] = React.useState(false); - React.useEffect(() => { - const childIds = []; - - React.Children.forEach(children, (child) => { - if (React.isValidElement(child) && child.props.nodeId) { - childIds.push(child.props.nodeId); - } - }); - if (arrayDiff(prevChildIds.current, childIds)) { - nodeMap.current[-1] = { parent: null, children: childIds }; - - childIds.forEach((id, index) => { - if (index === 0) { - setTabbable(id); - } - }); - visibleNodes.current = nodeMap.current[-1].children; - prevChildIds.current = childIds; - setChildrenCalculated(true); - } - }, [children]); - React.useEffect(() => { - const buildVisible = (nodes) => { - let list = []; - for (let i = 0; i < nodes.length; i += 1) { - const item = nodes[i]; - list.push(item); - const childs = nodeMap.current[item].children; - if (isExpanded(item) && childs) { - list = list.concat(buildVisible(childs)); - } + setTabbable((oldTabbable) => { + if (!oldTabbable) { + return descendants[0] ? descendants[0].id : null; } - return list; - }; - if (childrenCalculated) { - visibleNodes.current = buildVisible(nodeMap.current[-1].children); - } - }, [expanded, childrenCalculated, isExpanded, children]); + return oldTabbable; + }); + }, [descendants]); const noopSelection = () => { return false; }; - const [descendants, setDescendants] = useDescendantsInit(); - return ( { return d.element !== previousDescendants?.[i]?.element; }); @@ -96,38 +96,25 @@ export function useDescendant(descendant) { ...Object.values(descendant), ]); - return index; + return { parentId, index }; } export function useDescendantsInit() { return React.useState([]); } -export function useDescendants() { - return React.useContext(DescendantContext).descendants; -} - export function DescendantProvider(props) { - const { children, items, set } = props; + const { children, items, set, id } = props; const registerDescendant = React.useCallback( - ({ element, index: explicitIndex, ...rest }) => { + ({ element, ...rest }) => { if (!element) { return; } set((oldItems) => { let newItems; - if (explicitIndex != null) { - newItems = [ - ...oldItems, - { - ...rest, - element, - index: explicitIndex, - }, - ]; - } else if (oldItems.length === 0) { + if (oldItems.length === 0) { // If there are no items, register at index 0 and bail. newItems = [ ...oldItems, @@ -210,8 +197,9 @@ export function DescendantProvider(props) { descendants: items, registerDescendant, unregisterDescendant, + parentId: id, }), - [items, registerDescendant, unregisterDescendant], + [items, registerDescendant, unregisterDescendant, id], ); return {children}; @@ -219,6 +207,7 @@ export function DescendantProvider(props) { DescendantProvider.propTypes = { children: PropTypes.node, + id: PropTypes.string, items: PropTypes.array, set: PropTypes.func, }; From 3edb4e42cf09ac02fc747134e55cb6ca894e0bba Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Fri, 26 Jun 2020 00:08:32 +0100 Subject: [PATCH 3/8] Re-purpose bug test --- .../material-ui-lab/src/TreeView/TreeView.test.js | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-) diff --git a/packages/material-ui-lab/src/TreeView/TreeView.test.js b/packages/material-ui-lab/src/TreeView/TreeView.test.js index 056a9fcbc9704d..7169af632ce66e 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.test.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.test.js @@ -56,9 +56,7 @@ describe('', () => { ); }); - // 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', () => { + it('should not crash when unmounting with duplicate ids', () => { const CustomTreeItem = () => { return ; }; @@ -89,16 +87,7 @@ describe('', () => { expect(() => { screen.getByRole('button').click(); - }).toErrorDev([ - 'RangeError: Maximum call stack size exceeded', - 'The above error occurred in the component', - ]); - - const { - current: { errors }, - } = errorRef; - expect(errors).to.have.length(1); - expect(errors[0].toString()).to.include('RangeError: Maximum call stack size exceeded'); + }).not.toErrorDev(); }); }); From 8f3bccd62ba7e6250267862218075fd6d6653702 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Fri, 26 Jun 2020 01:19:35 +0100 Subject: [PATCH 4/8] Use #20147 and modify slightly --- .../material-ui-lab/src/TreeItem/TreeItem.js | 4 ++- .../src/TreeItem/TreeItem.test.js | 35 ++++++++++--------- .../material-ui-lab/src/TreeView/TreeView.js | 12 ++++--- .../src/TreeView/descendants.js | 4 +++ 4 files changed, 33 insertions(+), 22 deletions(-) diff --git a/packages/material-ui-lab/src/TreeItem/TreeItem.js b/packages/material-ui-lab/src/TreeItem/TreeItem.js index 6fe6750a78f872..bffe020cc33dd1 100644 --- a/packages/material-ui-lab/src/TreeItem/TreeItem.js +++ b/packages/material-ui-lab/src/TreeItem/TreeItem.js @@ -343,11 +343,13 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) { }; React.useEffect(() => { + // 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, }); return () => { @@ -356,7 +358,7 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) { } return undefined; - }, [registerNode, unregisterNode, parentId, index, nodeId]); + }, [registerNode, unregisterNode, parentId, index, nodeId, expandable]); React.useEffect(() => { if (mapFirstChar && label) { diff --git a/packages/material-ui-lab/src/TreeItem/TreeItem.test.js b/packages/material-ui-lab/src/TreeItem/TreeItem.test.js index 04b2a712c666cd..347c24231fb0f2 100644 --- a/packages/material-ui-lab/src/TreeItem/TreeItem.test.js +++ b/packages/material-ui-lab/src/TreeItem/TreeItem.test.js @@ -837,8 +837,9 @@ describe('', () => { describe('asterisk key interaction', () => { it('expands all siblings that are at the same level as the current node', () => { + const toggleSpy = spy(); const { getByTestId } = render( - + @@ -862,6 +863,8 @@ describe('', () => { fireEvent.keyDown(getByTestId('one'), { key: '*' }); + expect(toggleSpy.args[0][1]).to.have.length(3); + 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'); @@ -989,27 +992,27 @@ describe('', () => { 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); 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 }); @@ -1018,7 +1021,7 @@ describe('', () => { 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', () => { @@ -1036,11 +1039,11 @@ describe('', () => { 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', () => { @@ -1066,12 +1069,12 @@ describe('', () => { 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', () => { @@ -1178,11 +1181,11 @@ describe('', () => { 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', () => { @@ -1236,7 +1239,7 @@ describe('', () => { 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); }); }); @@ -1317,7 +1320,7 @@ describe('', () => { 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', () => { @@ -1334,7 +1337,7 @@ describe('', () => { 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); }); }); }); diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js index 99773af868b86d..beea03ab330f84 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.js @@ -94,6 +94,8 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { [expanded], ); + const isExpandable = React.useCallback((id) => nodeMap.current[id]?.expandable, []); + const isSelected = React.useCallback( (id) => (Array.isArray(selected) ? selected.indexOf(id) !== -1 : selected === id), [selected], @@ -295,7 +297,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { newExpanded = expanded.filter((id) => id !== value); setTabbable((oldTabbable) => { const map = nodeMap.current[oldTabbable]; - if (oldTabbable && map && map.parentId === value) { + if (oldTabbable && map?.parentId === value) { return value; } return oldTabbable; @@ -315,7 +317,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { const map = nodeMap.current[id]; const siblings = getChildren(map.parentId); - const diff = siblings.filter((child) => !isExpanded(child)); + const diff = siblings.filter((child) => isExpandable(child) && !isExpanded(child)); const newExpanded = expanded.concat(diff); @@ -495,9 +497,9 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { * Mapping Helpers */ const registerNode = React.useCallback((node) => { - const { id, index, parentId } = node; + const { id, index, parentId, expandable } = node; - nodeMap.current[id] = { id, index, parentId }; + nodeMap.current[id] = { id, index, parentId, expandable }; }, []); const getNodesToRemove = React.useCallback((id) => { @@ -555,7 +557,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { React.useEffect(() => { setTabbable((oldTabbable) => { if (!oldTabbable) { - return descendants[0] ? descendants[0].id : null; + return descendants[0]?.id; } return oldTabbable; diff --git a/packages/material-ui-lab/src/TreeView/descendants.js b/packages/material-ui-lab/src/TreeView/descendants.js index 1b106335abeeb7..872cfb123c170a 100644 --- a/packages/material-ui-lab/src/TreeView/descendants.js +++ b/packages/material-ui-lab/src/TreeView/descendants.js @@ -1,6 +1,10 @@ import * as React from 'react'; import PropTypes from 'prop-types'; +/** Credit: https://github.com/reach/reach-ui/blob/86a046f54d53b6420e392b3fa56dd991d9d4e458/packages/descendants/README.md + * Modified slightly to suit our purposes. + */ + // To replace with .findIndex() once we stop IE 11 support. function findIndex(array, comp) { for (let i = 0; i < array.length; i += 1) { From 753fece96b494e425d0df14ee58eabe282f84d16 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sun, 28 Jun 2020 20:53:42 +0100 Subject: [PATCH 5/8] olivier review --- .../src/TreeView/descendants.js | 27 +++++++++---------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/packages/material-ui-lab/src/TreeView/descendants.js b/packages/material-ui-lab/src/TreeView/descendants.js index 872cfb123c170a..87bad1331ab1ea 100644 --- a/packages/material-ui-lab/src/TreeView/descendants.js +++ b/packages/material-ui-lab/src/TreeView/descendants.js @@ -77,19 +77,24 @@ export function useDescendant(descendant) { // We also need to re-register descendants any time ANY of the other // descendants have changed. My brain was melting when I wrote this and it - // feels a little off, but checking in render and usin // effect's dependency array works well enough.g the result in the - const someDescendantsHaveChanged = descendants.some((d, i) => { - return d.element !== previousDescendants?.[i]?.element; + // feels a little off, but checking in render and using the result in the + // effect's dependency array works well enough. + const someDescendantsHaveChanged = descendants.some((newDescendant, position) => { + return newDescendant.element !== previousDescendants?.[position]?.element; }); // Prevent any flashing useEnhancedEffect(() => { - if (!descendant.element) forceUpdate({}); + if (!descendant.element) { + forceUpdate({}); + } registerDescendant({ ...descendant, index, }); - return () => unregisterDescendant(descendant.element); + return () => { + unregisterDescendant(descendant.element); + }; // eslint-disable-next-line react-hooks/exhaustive-deps }, [ registerDescendant, @@ -174,11 +179,7 @@ export function DescendantProvider(props) { return newItems.map((item, index) => ({ ...item, index })); }); }, - // set is a state setter initialized by the useDescendants hook. - // We can safely ignore the lint warning here because it will not change - // between renders. - // eslint-disable-next-line react-hooks/exhaustive-deps - [], + [set], ); const unregisterDescendant = React.useCallback( @@ -189,11 +190,7 @@ export function DescendantProvider(props) { set((oldItems) => oldItems.filter((item) => element !== item.element)); }, - // set is a state setter initialized by the useDescendants hook. - // We can safely ignore the lint warning here because it will not change - // between renders. - // eslint-disable-next-line react-hooks/exhaustive-deps - [], + [set], ); const value = React.useMemo( From fceb38dd186aa3369197132a3eb25b7e81420f5a Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Wed, 1 Jul 2020 23:18:03 +0100 Subject: [PATCH 6/8] sebastian's review --- .../material-ui-lab/src/TreeView/TreeView.js | 71 ++++++++++--------- .../src/TreeView/descendants.js | 5 +- 2 files changed, 39 insertions(+), 37 deletions(-) diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js index beea03ab330f84..59ca849255d2de 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.js @@ -15,18 +15,6 @@ export const styles = { }, }; -// To replace with Object.values() once we stop IE 11 support. -function objectValues(obj) { - const res = []; - // eslint-disable-next-line no-restricted-syntax - for (const i in obj) { - if (obj.hasOwnProperty(i)) { - res.push(obj[i]); - } - } - return res; -} - const findNextFirstChar = (firstChars, startIndex, char) => { for (let i = startIndex; i < firstChars.length; i += 1) { if (char === firstChars[i]) { @@ -80,11 +68,18 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { state: 'selected', }); - const getChildren = (id) => - objectValues(nodeMap.current) - .filter((node) => node.parentId === id) - .sort((a, b) => a.index - b.index) - .map((child) => child.id); + const getChildrenIds = (id) => { + const childIds = []; + + Object.keys(nodeMap.current).forEach((key) => { + const value = nodeMap.current[key]; + if (value.parentId === id) { + childIds.splice(value.index, 0, value.id); + } + }); + + return childIds; + }; /* * Status Helpers @@ -110,13 +105,13 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { const getNextNode = (id) => { // If expanded get first child - if (isExpanded(id) && getChildren(id).length > 0) { - return getChildren(id)[0]; + if (isExpanded(id) && getChildrenIds(id).length > 0) { + return getChildrenIds(id)[0]; } // Try to get next sibling const node = nodeMap.current[id]; - const siblings = getChildren(node.parentId); + const siblings = getChildrenIds(node.parentId); const nextSibling = siblings[siblings.indexOf(id) + 1]; @@ -127,7 +122,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { // try to get parent's next sibling const parent = nodeMap.current[node.parentId]; if (parent) { - const parentSiblings = getChildren(parent.parentId); + const parentSiblings = getChildrenIds(parent.parentId); return parentSiblings[parentSiblings.indexOf(parent.id) + 1]; } return null; @@ -135,34 +130,44 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { const getPreviousNode = (id) => { const node = nodeMap.current[id]; - const siblings = getChildren(node.parentId); + const siblings = getChildrenIds(node.parentId); const nodeIndex = siblings.indexOf(id); if (nodeIndex === 0) { return node.parentId; } - // start with previous node let currentNode = siblings[nodeIndex - 1]; - while (isExpanded(currentNode) && getChildren(currentNode).length > 0) { - currentNode = getChildren(currentNode).pop(); + while (isExpanded(currentNode) && getChildrenIds(currentNode).length > 0) { + currentNode = getChildrenIds(currentNode).pop(); } return currentNode; }; const getLastNode = () => { - let lastNode = getChildren(null).pop(); + let lastNode = getChildrenIds(null).pop(); while (isExpanded(lastNode)) { - lastNode = getChildren(lastNode).pop(); + lastNode = getChildrenIds(lastNode).pop(); } return lastNode; }; - const getFirstNode = () => getChildren(null)[0]; + const getFirstNode = () => getChildrenIds(null)[0]; const getParent = (id) => nodeMap.current[id].parentId; - const getOrder = (nodeAId, nodeBId) => { + /** + * This is used to determine the start and end of a selection range so + * we can get the nodes between the two border nodes. + * + * It finds the nodes' common ancestor using + * a naive implementation of a lowest common ancestor algorithm + * (https://en.wikipedia.org/wiki/Lowest_common_ancestor) and + * then compares the ancestors below it that are also above the + * given nodes so we can compare their indexes to work out which + * node is higher in the tree view. + */ + const getOrderOfNodesInTree = (nodeAId, nodeBId) => { if (nodeAId === nodeBId) { return [nodeAId, nodeBId]; } @@ -207,7 +212,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { } const commonAncestor = aAncestorIsCommon ? aAncestor : bAncestor; - const ancestorFamily = getChildren(commonAncestor); + const ancestorFamily = getChildrenIds(commonAncestor); const aSide = aFamily[aFamily.indexOf(commonAncestor) - 1]; const bSide = bFamily[bFamily.indexOf(commonAncestor) - 1]; @@ -218,7 +223,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { }; const getNodesInRange = (a, b) => { - const [start, end] = getOrder(a, b); + const [start, end] = getOrderOfNodesInTree(a, b); const nodes = [start]; let current = start; @@ -315,7 +320,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { const expandAllSiblings = (event, id) => { const map = nodeMap.current[id]; - const siblings = getChildren(map.parentId); + const siblings = getChildrenIds(map.parentId); const diff = siblings.filter((child) => isExpandable(child) && !isExpanded(child)); @@ -507,7 +512,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { const nodes = []; if (map) { nodes.push(id); - const childNodes = getChildren(id); + const childNodes = getChildrenIds(id); if (childNodes.length > 0) { nodes.concat(childNodes); childNodes.forEach((node) => { diff --git a/packages/material-ui-lab/src/TreeView/descendants.js b/packages/material-ui-lab/src/TreeView/descendants.js index 87bad1331ab1ea..88a98f91eb953e 100644 --- a/packages/material-ui-lab/src/TreeView/descendants.js +++ b/packages/material-ui-lab/src/TreeView/descendants.js @@ -16,10 +16,7 @@ function findIndex(array, comp) { return -1; } -const useEnhancedEffect = - typeof window !== 'undefined' && process.env.NODE_ENV !== 'test' - ? React.useLayoutEffect - : React.useEffect; +const useEnhancedEffect = typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffect; const DescendantContext = React.createContext({}); From 3645d2049b0b76f931724c2b30af298660bd89e9 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Thu, 2 Jul 2020 21:10:47 +0100 Subject: [PATCH 7/8] Re-word --- .../material-ui-lab/src/TreeView/TreeView.js | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js index 59ca849255d2de..e54c4ca968ee8b 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.js @@ -89,7 +89,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { [expanded], ); - const isExpandable = React.useCallback((id) => nodeMap.current[id]?.expandable, []); + const isExpandable = React.useCallback((id) => nodeMap.current[id].expandable, []); const isSelected = React.useCallback( (id) => (Array.isArray(selected) ? selected.indexOf(id) !== -1 : selected === id), @@ -162,12 +162,15 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { * * It finds the nodes' common ancestor using * a naive implementation of a lowest common ancestor algorithm - * (https://en.wikipedia.org/wiki/Lowest_common_ancestor) and - * then compares the ancestors below it that are also above the - * given nodes so we can compare their indexes to work out which - * node is higher in the tree view. + * (https://en.wikipedia.org/wiki/Lowest_common_ancestor). + * Then compares the ancestor's 2 children that are ancestors of nodeA and NodeB + * so we can compare their indexes to work out which node comes first in a depth first search. + * (https://en.wikipedia.org/wiki/Depth-first_search) + * + * Another way to put it is which node is shallower in a trémaux tree + * https://en.wikipedia.org/wiki/Tr%C3%A9maux_tree */ - const getOrderOfNodesInTree = (nodeAId, nodeBId) => { + const findOrderInTremauxTree = (nodeAId, nodeBId) => { if (nodeAId === nodeBId) { return [nodeAId, nodeBId]; } @@ -222,13 +225,13 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { : [nodeBId, nodeAId]; }; - const getNodesInRange = (a, b) => { - const [start, end] = getOrderOfNodesInTree(a, b); - const nodes = [start]; + const getNodesInRange = (nodeA, nodeB) => { + const [first, last] = findOrderInTremauxTree(nodeA, nodeB); + const nodes = [first]; - let current = start; + let current = first; - while (current !== end) { + while (current !== last) { current = getNextNode(current); nodes.push(current); } From e120da932f7280529d00b203b640a52d549f3d97 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Fri, 3 Jul 2020 01:38:53 +0100 Subject: [PATCH 8/8] More readable? --- .../material-ui-lab/src/TreeView/TreeView.js | 24 ++++++++----------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/packages/material-ui-lab/src/TreeView/TreeView.js b/packages/material-ui-lab/src/TreeView/TreeView.js index e54c4ca968ee8b..c56271124fc8b0 100644 --- a/packages/material-ui-lab/src/TreeView/TreeView.js +++ b/packages/material-ui-lab/src/TreeView/TreeView.js @@ -68,19 +68,15 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { state: 'selected', }); - const getChildrenIds = (id) => { - const childIds = []; - - Object.keys(nodeMap.current).forEach((key) => { - const value = nodeMap.current[key]; - if (value.parentId === id) { - childIds.splice(value.index, 0, value.id); - } - }); - - return childIds; - }; - + // Using Object.keys -> .map to mimic Object.values we should replace with Object.values() once we stop IE 11 support. + const getChildrenIds = (id) => + Object.keys(nodeMap.current) + .map((key) => { + return nodeMap.current[key]; + }) + .filter((node) => node.parentId === id) + .sort((a, b) => a.index - b.index) + .map((child) => child.id); /* * Status Helpers */ @@ -401,7 +397,7 @@ const TreeView = React.forwardRef(function TreeView(props, ref) { }; const handleMultipleSelect = (event, value) => { - let newSelected = []; + let newSelected; if (selected.indexOf(value) !== -1) { newSelected = selected.filter((id) => id !== value); } else {