brimdata / react-arborist

The complete tree view component for React
MIT License
3.04k stars 139 forks source link

Manipulating state of selected ids from outside of the Tree component is causing infinite loops. #176

Open adrianwysocki1993 opened 1 year ago

adrianwysocki1993 commented 1 year ago

Hey Dear Community! I have case where Im stuck.

What I want to achieve? a) Track state of selectedIds outside of the tree component b) Manipulate state of selectedIds from outside of the component and also through clicking on the tree nodes independently

What is my problem? I can not manipulate state selectedIds from outside. Using useEffect with tree?.current?.setSelection (see the code) and also passing onSelect to the tree component is causing infinite loop. Do you have some ideas what can I do? Why calling tree?.current?.setSelection is also triggering onSelect function? In my opinion calling these methods should be independent.

What I propose to change? a) Selected ids elements should be easily changed by just passing array of selected elements to the Tree component like we do with data. b) Calling tree?.current?.setSelection should not trigger onSelect function as one change can come from outside of the component and another from clicking on a tree node.

import React, { useEffect, useRef } from "react";

import type { NodeApi } from "react-arborist";
import { Tree as TreeArborist } from "react-arborist";
import { Node } from "./Node";
import type { TreeNode } from "./Node";

export const Tree = ({
  onSelectionChange,
  selectedIds,
  sourceData,
}) => {
  const tree = useRef();

  useEffect(() => {
    tree?.current?.setSelection({
      mostRecent: null,
      anchor: null,
      ids: selectedIds,
    });
  }, [selectedIds]);

  const onSelect = (idsMap: NodeApi<TreeNode>[]) => {
    const ids = Array.from(idsMap.values()).map((node) => node.id);
    onSelectionChange(ids);
  };

  return (
    <TreeArborist
      data={sourceData}
      disableMultiSelection={false}
      onSelect={onSelect}
      ref={tree}
    >
      {Node}
    </TreeArborist>
  );
};

@jameskerr

adrianwysocki1993 commented 1 year ago

Hey! I discovered another thing, using tree?.current?.setSelection in useEffect is making multi selection through shift broken as Im not passing anchor element and most recent.

adrianwysocki1993 commented 1 year ago

I managed to hack the problem:

import React, { useEffect, useRef } from "react";

import type { NodeApi } from "react-arborist";
import { Tree as TreeArborist } from "react-arborist";
import { Node } from "./Node";
import type { TreeNode } from "./Node";

export const Tree = ({
  onSelectionChange,
  selectedIds,
  sourceData,
}) => {
  const tree = useRef();
  const numberOfRendersOnSelectRef = useRef({ number: 0 });

  useEffect(() => {
    tree?.current?.setSelection({
      mostRecent: null,
      anchor: null,
      ids: selectedIds,
    });
  }, [selectedIds]);

  const onSelect = (idsMap: NodeApi<TreeNode>[]) => {
    numberOfRendersOnSelectRef.current.number =
      numberOfRendersOnSelectRef.current.number + 1;

    if (numberOfRendersOnSelectRef.current.number <= 2) {
      return;
    }

    const ids = Array.from(idsMap.values()).map((node) => node.id);
    onSelectionChange(ids);
  };

  return (
    <TreeArborist
      data={sourceData}
      disableMultiSelection={false}
      onSelect={onSelect}
      ref={tree}
    >
      {Node}
    </TreeArborist>
  );
};
jameskerr commented 1 year ago

Yes, this is a feature gap at the moment. I have a solution for this in mind where you can change the selection from the outside in a declarative way. The design idea is written about here: https://www.jameskerr.blog/posts/partially-controlled-react-components/

You'll need to set the anchor and mostRecent id if you want shift+up/down selection to still work. You could just set the anchor and mostRecent to the last id in your array.

AndrejGajdos commented 8 months ago

@jameskerr it would be great to be able to control selected state.

There is another issue where onSelect is called on initial render. You can see it if you add the onSelect callback to one of your demos. @adrianwysocki1993 is this the reason why you have numberOfRendersOnSelectRef in your code?

Another thing is that onSelect is not called if you unselect an item by shift+click on selected item. I guess this use case is not implemented in @adrianwysocki1993 snippet? It seems like it's not possible to implement controlled selection state with multiselect action?