v2 restructure using TreeModel to hold state

[worthlutz]

This restructure moves the tree state into a TreeModel. CheckboxTree and other React components display the tree.

CheckboxTree is an uncontrolled component. The state is stored in a context and supplied by the CheckboxTreeProvider. This can be seen in the BasicExample. All the examples work including a new one displaying the additions of radio button node groups, RadioButtonExample. This is an addition I had working on a fork of v1.6.

The initial tree which was input to CheckboxTree as the nodes prop previously is now the initialTreeState prop. The state formally kept in the checked and expanded arrays is now input through the initialTreeState object.

The onCheck and onExpand handlers are passed a changedNodeKey and an instance of TreeModel,. This allows an application to access the tree and do whatever is required based on the changed node. For example:

    const onCheck = (changedNodeKey, newTree) => {
        const changedNode = newTree.getNode(changedNodeKey);
        console.log(`changed node = ${changedNode.label}`);
        console.log(newTree.getChecked());
    };

Note that the TreeModel has public methods which allow access to the tree and its nodes. A good example of this is in the FilterExample where the TreeModel.filter method is used. Excerpt below:

    const [filterText, setFilterText] = useState('');

    // function to determine if node meets filter test
    // returns Boolean
    const filterTest = (node) => (
        // node's label matches the search string
        node.label.toLocaleLowerCase().indexOf(filterText.toLocaleLowerCase()) > -1
    );

    const applyFilter = () => {
        treeModel.filter(filterTest);
        setActiveFilter(filterText);
    };

Not all the tests are fixed yet and I'm working on the documentation now.

Questions, comments, criticisms and suggestions requested.

[worthlutz]

Utils have been moved to public methods of TreeModel.

[worthlutz]

Custom Label Example was added in the last commit along with the changes needed for it to work. This is something I have needed in a project of mine. I think I have it working with reasonable props to make it work.

[worthlutz]

@jakezatecky This last commit gets this pull request in pretty good shape. I've been down lots of rabbit holes and tried many options some of which turned out to be illogical. I feel good with the results so far.

• worked on all examples and added some new ones.
• fixed all tests and added some new ones.
• started work on the documentation (I need to do more work on it)

I have a version of this running in production now. So far so good. :)

I think the best way to start understanding the changes is to look at all of the examples. Here are a few hints:

• The nodes prop is used only as the initial state of the TreeModel. Changing the nodes prop re-initializes the TreeModel state to the new data in nodes. The previous instance of TreeModel is discarded.
• In order to change the state in the TreeModel programmatically the public methods of the TreeModel should be used. (see FilterExample)
• TreeModel is stored in the CheckboxTreeContext and provided to the components by the <CheckboxTreeProvider>. The provider can be moved up in the component tree to allow state to be saved between mountings of the CheckboxTree.
• Access to the TreeModel instance is through the useCheckboxTree hook if changes are to be made programmatically.
• The TreeModel instance is also passed to CheckboxTree methods like onCheck as the second argument (see BasicExample). This allows access to the changed node and its children and parents.
• All nodes have a consistent set of properties as defined in NodeModel as well as any other properties which exist in the nodes prop. There is a TreeModel method to change these other properties in a node. Should this method not be allow to change the NodeModel defined properties? This seems prudent but I have not done this yet.

Please let me know what questions, comments or criticisms you have when you find some time to review it.

[jakezatecky]

Thank you very much for your continued work on this. I admit and apologize that I have been sorely lacking in providing commentary over the last several weeks.

I intend to give the most recent changes a good review within the next few days.

[jakezatecky]

getCheckedArray does not appear to be a method of TreeModel.

[worthlutz]

fixed : fix incorrect method
a27a085

[jakezatecky]

Since we are already calling getNode, it would be cleaner for the user if node gets returned to their onCheck rather than having them call newTree.getNode(nodeKey) again. See main review comment for more details.

[worthlutz]

That is a reasonable request. I think I may have been worried about a node being in a closure and returning a stale node to the event handlers.

[jakezatecky]

Hello @worthlutz . Thanks for all the effort you have put into this PR! I really appreciate your work and tolerance for my less than timely responses. I finally spent some time to review many of the changes.

I find your approach interesting. Although a breaking change, I love merging checked and expanded into the overall node state, given the complications of maintaining these separately. I also like providing the full tree to handler functions such that users can do whatever with it as they please. Further, I like the idea of using modern hooks to expose elements of the tree where it makes sense. I also like that this iteration is overall simpler from the user perspective than what we had discussed before.

However, I do have some general items of discussion after reviewing many of the changes. The main discussion surrounds the context/provider.

• I think onCheck/onExpand should provide the actual node in the first argument without requiring the user call getNode(key). For example, looking at onCheckHandler, I see TreeModel.toggleChecked already calls this.getNode(nodeKey). Given that the internal code is already fetching the node itself, I think we should pass it straight back to the user, rather them making the call again.
• I understand that relying more on the TreeModel resulted in the component now being uncontrolled vs. controlled. The advantage of being uncontrolled is that the internal code is simpler. I am not necessarily opposed to making the tree uncontrolled, but I'll admit that I like to control state rather than having an underlying component do it in some instances. I can be persuaded to keep everything uncontrolled, however, as this leads into a larger point.
• One of my main concerns is that the overall API exposed to users is a bit more complicated with the introduction of the CheckboxTreeProvider. From the BasicExample, it is not immediately clear to users why they must wrap the component around in a provider. I understand that the documentation mentions this, but it does increase user complexity (and boilerplate code).
  • I understand that by having a context, the tree state can be accessed elsewhere, and that does have some utility. I often see providers/context used with theming, less so with a library's data layer, but I would be interested if there are examples of other libraries doing this. I am also unsure how many users would take advantage of this.
  • The extra code for users and explanatory text in the documentation somewhat nudges me against using the context approach. Considering that TreeModel already works with an underlying nodes variable for manipulation, I think I would prefer that nodes (or tree) maintain all state and be updated through onChange. I think this would be simpler from the user perspective. Do you see any issues with this approach? We could either use nodes or tree, but I'm going with tree here. The code below is controlled, but we could make it uncontrolled, too.

const [tree, setTree] = useState(nodes);

// Legacy methods
const onCheck = (node, tree) => {
    console.log(node, tree.getChecked());
};
const onExpand = (node, tree) => {
    console.log(node, tree.getExpanded());
};

// Main state method
const onChange = (tree) => {
    setTree(tree);
};

return <CheckboxTree tree={tree} onChange={onChange} onCheck={onCheck} onExpand={onExpand} />;

* Alternatively, we could have the user generate an initial `treeModel` through a helpful function and pass _that_ to the `CheckboxTree` component rather than `nodes`. A potential advantage of this is that users could mutate the model directly, such as calling `treeModel.toggleChecked` before the even the first render!
* (Minor) We could support both uncontrolled and controlled states by having `defaultNodes` for uncontrolled state and `nodes` for controlled. Users would choose one or the other. Some users prefer uncontrolled state, and some prefer controlled state, and I think we can support both with relative ease, but we can stick to one during the development phase to simplify things.

I really like the idea of relying more on a TreeModel instance that you have put forward. Here are some additional methods I would add later, but need not be in the PR now:

• setChecked/setExpanded: would traverse the tree and modify the state. The documentation would encourage users to set the state nodes directly, but these could help users of the previous version transition to the latest version without having to compile nodes directly.
• getNodes: This would be the inverse of initializing the state. This would be great for users to be able to recall later if we don't use nodes/tree and setNodes/setTree as a controlled component.

[worthlutz]

hI @jakezatecky. I've been on vacation and had some other issues to deal with thus my slow response. Thanks for your comments. I will digest them more this week and come back with some more thoughtful answers and reasoning than I can do quickly today.

I think your ideas are good and I had many of the same ideas as I tried one thing and then another. Eventually what I ended up with just evolved out of the process of trial and error. Now that it is working, changes will be easier to make.

[worthlutz]

hi @jakezatecky. I can get rid of the context used to save state and make the component a controlled component. This will remove the unneeded complexity I somehow decided was needed. The input prop will be a TreeModel instead of the nodeShape previously used. Instead of a helper function the nodes array will be input to the TreeModel' constructor. See new BasicExample` below.

All state is contained in the TreeModel and since the component will now be controlled that state can be pulled up high in the component tree if needed to maintain state when the CheckboxTree is mounted and unmounted. The method of pulling state up is now controlled by the developer using CheckboxTree and not required to be a context.

I have not looked at returning the node instead of the key yet.

More comments to follow as I review more.

import React, { useState } from 'react';
import CheckboxTree, { TreeModel } from 'react-checkbox-tree';

import { fileSystem as nodes } from './data'; // or from server request in hook

const initialTree = new TreeModel(nodes);

function BasicExample() {
    const [tree, setTree] = useState(initialTree);

    const onChange = (newTree) => {
        setTree(newTree);
    };

    const onCheck = (changedNodeKey, newTree) => {
        const changedNode = newTree.getNode(changedNodeKey);
    };

    const onExpand = (changedNodeKey, newTree) => {
        const changedNode = newTree.getNode(changedNodeKey);
    };

    return (
        <CheckboxTree
            tree={tree}
            onChange={onChange}
            onCheck={onCheck}
            onExpand={onExpand}
        />
    );
}

I have made the code changes and have this working in all the examples but have to do some more work on the tests to fix them. I will also need to work on the documentation to reflect these changes.

[worthlutz]

Hi @jakezatecky. The latest commits remove the context used to save state. I'm still not sure why I ended up with that. All examples and tests work. None of your other comments have been addressed yet.

[worthlutz]

Hi @jakezatecky,

I have reviewed you comments and agree with your views. I have made the changes and have the code in a pretty good state. The documentation is not finished but it should be in a reasonable state also. I have not added the additional methods to TreeModel but think they are a good idea. I think at this point another review is warranted.

Also here is a list of questions/comments I think need addressing:

• Given that both CheckboxTree and TreeModel need to be imported to use CheckboxTree, do you have a preference?

import CheckboxTree, { TreeModel } from 'react-checkbox-tree'
                        --or--
import { CheckboxTree, TreeModel } from 'react-checkbox-tree'

• I'm not familiar with the contents of index.d.ts and have not touched it. I could guess based upon what it looks like but...

• The noCascade prop has been split into noCascadeChecks and noCascadeDisabled. In v1 of Checkboxtree it was not documented that the noCascade prop was used for disabled but was used that way in the code. I made them different to better define what is happening. There are some problems with noCascadeDisabled which are discussed several bullets down.

• Note: The disabled prop is passed to CheckboxTree and disables the entire tree. It has nothing to do with the disabled state of individual nodes or noCascadeDisabled.

• I have removed some props from CheckboxTree and added them as an options object to be input to the TreeModel. These are properties which are used in TreeModel almost exclusively. Review this in the docs and determine if you think this is the way to go. (see also more discussion in the major bullets below)

  • checkModel is only used by the TreeModel.getChecked() method.
  • optimisticToggle is only used by TreeModel and not CheckboxTree
  • noCascadeChecks is used by CheckboxTree only in CheckboxIcon. This prop is gotten from the TreeModel instance and is passed to TreeNode and then down through props to CheckBoxIcon . It is not needed as a prop to CheckboxTree.
  • noCascadeDisabled is used by both TreeModel and CheckboxTree. Giving it to TreeModel first allows TreeModel to use it in the constructor. It is then extracted by CheckboxTree for use there.

• noCascadeDisabled defaults to false and determines if child nodes take on the disabled property of an ancestor. This is currently working but is handled by both CheckboxTree and TreeModel. I'm not sure of the best way to handle the disabled state of nodes.

  • One way is to have TreeModel handle the state and have each node have it's disabled property set. The advantages of this is that all nodes have their own disabled property and when inspecting the node, one can see that state. The disadvantage is that the initial state must be consistent and correct and/or the code must put and keep all nodes in a consistent state. Changing a parent's disabled state will need to cascade down and all child nodes will need changing in the TreeModel.
  • The other idea is to only change the state of the parent and let CheckboxTree handle the disabled state of the child nodes. In other words, a node is disabled if it has a disabled ancestor or it is disabled itself. CheckboxTree currently handles this situation. The advantage of this method is that if you disable a node, all child nodes below it are disabled. Then if you reenable that node, all child nodes below return to their previous state (disabled or not). The disabled state will not be stored in each node; only those explicitly disabled. To determine if a node is disabled by inspection, the ancestors of that node will need to be inspected if the node is not explicitly disabled.

My choice on noCascadeDisabled is to use the second method described above. This removes the requirement that all disabled nodes have a disabled property. Only nodes explicitly disabled will need a disabled property. CheckboxTree will properly cascade down the child nodes of a disabled node and disable them. Or not if noCascadeDisabled=true.

Let me know if the above discussion is not clear. I'm looking forward hearing your comments.

[worthlutz]

The latest commit defines one method of handling the disabled property of nodes and how cascade works with respect to the disabled property.

I've changed the noCascadeDisabled to disabledCascade to get rid of all the !noCascadeDisabled double negatives for easier reading code. (I want to do the same with noCascadeChecks later)

The TreeModel option disabledCascade defaults to true meaning cascade disabled to child nodes.

NOTE: child nodes do not have their disabled property set due to cascading in the TreeModel.

Nodes can be marked as disabled individually with the node disabled property when setting up the inital nodes array. The TreeModel constructor sets the disabled property on nodes as set in that inital nodes array. The disabledCascade option is not used in setting up the initial TreeModel as cascading is handled in CheckboxTree.

Disabling child nodes is handled by CheckboxTree.renderTreeNodes() during rendering. It disables all child nodes below a disabled node when disabledCascade=true regardless of the value of the disabled property of the child node.

When disabledCascade=false, whether a node is disabled or not depends upon the node's own disabled property.

TreeModel.toggleDisabled(nodeKey) will only toggle the disabled property in that single node. Child nodes will not have their disabled property changed.

TreeModel.getDisabled() returns the disabledArray of nodeKeys. disabledCascade determines which nodes are included in the array. If false only nodes with a disabled=true are returned.

It defaults to the value of disabledCascade in the TreeModel.options and can be overidden by passing a true|false argument to getDisabled.

const getDisabled(disabledCascade = this.options.disabledCascade) {}

Let me know what you think of this definition of disabled and disabledCascade.

---

The alternative to the above definition would be to handle all cascading in the TreeModel. When the initial state is converted in the TreeModel constructor, the disabledCascade property would be taken into account. The TreeModel would be made consistent with the disabled properties cascading or not.

Problems occur with this definition when a the value of disabledCascade changes. Changing disabledCascade from true to false, means that when a node is toggled only that node changes. If the value of disabledCascade were to change back to true the disabled state in the TreeModel would need to be checked to get it back into a correct state.

This seemed more difficult and confusing to me so I chose the other method.

---

Questions:

• Should getchecked()ignore disabled nodes when returning the checked array? I do not use this array so I have no clue. It's easy to do either way.

• Should toggleChecked(nodeKey) toggle a disabled node? I would assume not but thought I should ask. This will work easily to toggle if not disabled and can also respect the disabledCascade value.