Skip to content
This repository was archived by the owner on Sep 21, 2021. It is now read-only.

Commit 1ab7264

Browse files
committed
Unvirtualize the Tree component.
We were having an onScroll event that was making the console lag if you had a large number of ObjectInspector in it. None of the consumer of this Tree component take advantage of this so let's remove it. We might want to reintroduce some virtualization later, but maybe using the IntersectionObserver instead, since we can have variable height items. Also this PR breaks keyboard navigation, as if you navigate to an off-canvas item you won't be scrolled. I think this was already broken for variable height items though. This should be fixed in another PR with keeping in mind that this tree is not meant to be used as the single component of a container. At the moment, every consumer disable the keyboard navigation, so we don't put anyone at risk.
1 parent e2eda58 commit 1ab7264

File tree

1 file changed

+17
-52
lines changed
  • packages/devtools-components/src

1 file changed

+17
-52
lines changed

packages/devtools-components/src/tree.js

Lines changed: 17 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -334,10 +334,6 @@ const Tree = createClass({
334334
// isExpanded: item => item.expanded,
335335
isExpanded: PropTypes.func.isRequired,
336336

337-
// The height of an item in the tree including margin and padding, in
338-
// pixels.
339-
itemHeight: PropTypes.number.isRequired,
340-
341337
// Optional props
342338

343339
// The currently focused item, if any such item exists.
@@ -389,25 +385,16 @@ const Tree = createClass({
389385

390386
getInitialState() {
391387
return {
392-
scroll: 0,
393-
height: window.innerHeight,
394388
seen: new Set(),
395389
};
396390
},
397391

398392
componentDidMount() {
399-
window.addEventListener("resize", this._updateHeight);
400393
this._autoExpand();
401-
this._updateHeight();
402394
},
403395

404396
componentWillReceiveProps(nextProps) {
405397
this._autoExpand();
406-
this._updateHeight();
407-
},
408-
409-
componentWillUnmount() {
410-
window.removeEventListener("resize", this._updateHeight);
411398
},
412399

413400
_autoExpand() {
@@ -464,15 +451,6 @@ const Tree = createClass({
464451
}
465452
},
466453

467-
/**
468-
* Updates the state's height based on clientHeight.
469-
*/
470-
_updateHeight() {
471-
this.setState({
472-
height: this.refs.tree.clientHeight
473-
});
474-
},
475-
476454
/**
477455
* Perform a pre-order depth-first search from item.
478456
*/
@@ -555,22 +533,23 @@ const Tree = createClass({
555533
* The item to be focused, or undefined to focus no item.
556534
*/
557535
_focus(index, item) {
558-
if (item !== undefined) {
559-
const itemStartPosition = index * this.props.itemHeight;
560-
const itemEndPosition = (index + 1) * this.props.itemHeight;
561-
562-
// Note that if the height of the viewport (this.state.height) is less than
563-
// `this.props.itemHeight`, we could accidentally try and scroll both up and
564-
// down in a futile attempt to make both the item's start and end positions
565-
// visible. Instead, give priority to the start of the item by checking its
566-
// position first, and then using an "else if", rather than a separate "if",
567-
// for the end position.
568-
if (this.state.scroll > itemStartPosition) {
569-
this.refs.tree.scrollTop = itemStartPosition;
570-
} else if ((this.state.scroll + this.state.height) < itemEndPosition) {
571-
this.refs.tree.scrollTop = itemEndPosition - this.state.height;
572-
}
573-
}
536+
// TODO: Revisit how we should handle focus without having fixed item height.
537+
// if (item !== undefined) {
538+
// const itemStartPosition = index * this.props.itemHeight;
539+
// const itemEndPosition = (index + 1) * this.props.itemHeight;
540+
541+
// // Note that if the height of the viewport (this.state.height) is less than
542+
// // `this.props.itemHeight`, we could accidentally try and scroll both up and
543+
// // down in a futile attempt to make both the item's start and end positions
544+
// // visible. Instead, give priority to the start of the item by checking its
545+
// // position first, and then using an "else if", rather than a separate "if",
546+
// // for the end position.
547+
// if (this.state.scroll > itemStartPosition) {
548+
// this.refs.tree.scrollTop = itemStartPosition;
549+
// } else if ((this.state.scroll + this.state.height) < itemEndPosition) {
550+
// this.refs.tree.scrollTop = itemEndPosition - this.state.height;
551+
// }
552+
// }
574553

575554
if (this.props.onFocus) {
576555
this.props.onFocus(item);
@@ -584,19 +563,6 @@ const Tree = createClass({
584563
this._focus(0, undefined);
585564
},
586565

587-
/**
588-
* Fired on a scroll within the tree's container, updates
589-
* the stored position of the view port to handle virtual view rendering.
590-
*
591-
* @param {Event} e
592-
*/
593-
_onScroll: oncePerAnimationFrame(function (e) {
594-
this.setState({
595-
scroll: Math.max(this.refs.tree.scrollTop, 0),
596-
height: this.refs.tree.clientHeight
597-
});
598-
}),
599-
600566
/**
601567
* Handles key down events in the tree's container.
602568
*
@@ -767,7 +733,6 @@ const Tree = createClass({
767733
onKeyDown: this._onKeyDown,
768734
onKeyPress: this._preventArrowKeyScrolling,
769735
onKeyUp: this._preventArrowKeyScrolling,
770-
onScroll: this._onScroll,
771736
onFocus: ({nativeEvent}) => {
772737
if (focused || !nativeEvent || !this.refs.tree) {
773738
return;

0 commit comments

Comments
 (0)