refactor for v2 #147

worthlutz · 2019-05-14T19:53:27Z

@jakezatecky,
Here is a pull request with some breaking changes. I'm not sure you are interested but I thought I'd offer it up before forking off in this direction. I think it would make a good v2.

I have made the changes to branch v1.6-dev as this seemed to be the best plact to start. I will rebase it on master once v1.6 is merged.

The goals of this revision is to simplify the code and speed it up on large trees. It also adds RadioGroups as described in pull request #99. Comments made there have been taken into account.

Summary

The state is not kept in the checked and expanded arrays. Node state (checked, expanded and disabled) are stored in the node tree on each node object. This allows the removal of the flattened tree and the nodeModel. Note that the checked array is still available in the hidden input if the name prop is given.
The nodes prop of CheckboxTree now accepts either an object or an array. The array is the node array used in previous versions. The object is the root node of the tree whose children are the node array used previously. As CheckboxTree is a controlled component, the proper new nodes is sent as an argument to oncheck and onExpand to be stored in state and passed back as a new value for the nodes prop.
and a new object or array is returned. Only changed nodes are new.
The nodes object or array is treated as an immutable object. The objects returned are new objects with only the changed nodes being new objects.
The node returned by onCheck or onExpand is the node with updated properties and it also includes all properties from the original node. This fixes problems I was having where only some properties were returned in a node-like object. I have had problems with this in the past because upon checking a node something must happen which is based upon additional properties stored in the node.
The code only walks the node tree once, during render. It processes leaf nodes first during the tree walk so the proper checkstate and other properties can be calculated for each node. All defaults are calculated during this single walk through the tree.
Treenode now has a shouldComponentUpdate to bypass rendering when unnecessary. Also the handling of the icons prop has been changed to eliminate creating a new array every time before passing as a prop.
New PropsDemoExample demonstrates the function of the various props and various prop combinations. Please try this example and confirm that things work as expected.

There may be other changes I have forgotten to enumerate.

Please take a look at the examples and read through the CheckboxTree and TreeNode code and let me know if this interests you.

I welcome comments and criticism. :)

fix hidden input first go at radio groups fix NativeCheckbox to handle radioGroups fix node disabled add PropsDemo example clean up examples fix expandOnClick in PropsDemoExample fix CustomIconsExample fix icons prop problem & problem with updateParentNodes cleanup handling of icons prop add ability to input object instead of array to Nodes remove icons test from CustomIconsExample remove unneeded NodeModel add some radio nodes fix TreeNode toggleChecked fix class name change example titles fix object/array return in onCheck and onExpand

worthlutz · 2019-05-14T21:10:58Z

Tests have not been updated yet and thus fail...

This reverts commit 3dba178.

jakezatecky · 2019-05-23T20:44:09Z

Sorry for the radio silence and thanks for the PR. This includes a lot of sweeping changes that I will have to review. I plan to look it over on the weekend. Naturally, there will be a need for similar levels of automated tests.

I already have some preliminary thoughts, but this deserves a more complete review on my part to determine whether we achieve the same level of feature parity as before.

worthlutz · 2019-05-24T01:21:23Z

@jakezatecky I have not had a chance to complete the new tests yet. It is on my list. I look forward to your comments on this pull request.

I have a few ideas to clean up or simplify some more of this. One idea I've been thinking about is changing the onCheck and onExpand to a single function, onChange. This function needs to save the nodes object/array to state for both and may need to do some work for a 'check' change but probably needs nothing other than the state change for the onChange. The function can get a change type argument ('check' | 'expand'). I think this may simplify it a bit.

One other enhancement I am needing is an addition to the TreeNode. This would be an additional child component inside the TreeNode. This is a bit complicated as the TreeNodes are dynamically built from the input nodes object/array. This dynamic building of the tree is necessary for me as I am getting the nodes object array from a JSON config file. The additional child component will get its props from the node config. I would be interested in any ideas you have on this.

jakezatecky

Here is an initial review. Again, thank you for all the effort you have put into this!

I ran the changes locally and mostly liked what I saw. Naturally, the end result would need all tests to pass and I can help with that provided that the end result allows for the same set of options as previously allowed.

Beyond the comments on individual lines, I have a strong desire to still support an array-value system somewhere, even if it is offloaded to some helper function that the developer would invoke. The project that I had that initially necessitated that I role out this CheckboxTree component had great need of storing the an array of the selected values and repopulating the tree from those values. Naturally, the v2 changes for storing state in the nodes themselves runs counter to this. The values are already stored in the hidden input, I see, but recalling the state from those values is not quite supported. While you do not need to work on that, I think it may be prudent to add some helper functions to load the state from an array of values as well as serialize the values from the state.

jakezatecky · 2019-05-26T07:26:59Z

examples/src/js/BasicExample.js

-    onCheck(checked) {
-        this.setState({ checked });
+    onCheck = (node, nodes) => {
+        this.setState({ nodes });
    }


Suggested change

}

};

Any assignment operator should be terminated by a semicolon.

Will fix these. They are not caught by eslint.

jakezatecky · 2019-05-26T07:28:46Z

examples/src/js/BasicExample.js


        return (
            <CheckboxTree
-                checked={checked}
-                expanded={expanded}
+                style={{ flex: '1' }}


There is no style property for the main component and I would be extremely averse to adding such a property.

That style prop is not a <CheckboxTree> prop. It is a prop of React.component which checkbox tree extends. I understand you not wanting to use it. I have tried to modify the css in the actual component. I have used it in the demos for fast testing. I can clean this up and add separate css files for the demos.

jakezatecky · 2019-05-26T07:32:02Z

examples/src/js/BasicExampleObject.js

+import CheckboxTree from 'react-checkbox-tree';
+
+const initialNodes = {
+    label: 'Root',


I do not see the point in supporting a base object file. It makes the internals slightly more complicated and it is not too much to ask for the user just to have a top-level array. Also, it does not seem like this is actually doing much?

This is something I put in based upon my use and something I read in one of the issues somewhere. I will state my reasoning here and you can decide the path forward as this is your component.

In my use I may have several JSON configuration files which are used for input to the CheckTree. Each has a root node with a description of that tree. The User logs in and can choose from the configurations available to them. My current proposal allows passing this root node object in as the nodes prop. In this case, the root node is the same type object as as each child node object. I can achieve the same result by passing the root node's children array as you originally set it up.

I have also allowed the nodes prop to be passed in as an array. It is then put into a generated root object so that the processing could be handled the same as when a root object is passed in as described above.

One advantage of having the root object is that you can then add a prop, showRoot, to allow having a single node at the top of the tree. This allows you to collapse the tree to one line.

By defaulting showRoot to false you will have the same result as you originally provided. I have not yet added this prop but had intended to.

The showRoot prop will fix some of the concerns in issue #143. With showRoot === true and 'noCascade === false you do not need a 'checkAll or 'unCheckAll`. Just check or uncheck the root.

That is my case for adding the capability to input a root node. I have kept the ability to input an array of nodes for compatibility with previous versions. I can set it up with only the root node option, only the array of nodes option or both. You choose.

jakezatecky · 2019-05-26T07:37:39Z

examples/src/js/PropsDemoExample.js

+        // console.log('------------------------------------------------');
+
+        return (
+            <div style={{ display: 'flex' }}>


This is all very minor in comparison to more important changes, but I intensely dislike the use of inline styles. I somewhat like the idea of showcasing many of the features, but I much prefer semantic class names with appropriate styles in a stylesheet.

Do not spend any real effort on this piece because I am on the fence on whether to include this React file because I generally want the example files to be minimally small for users to be able to quickly understand, modify, and copy them.

I can follow your lead on inline styles. I mostly use them for testing as it is much quicker for me.

This comprehensive example has been very important to me for testing. I placed it at the top of the examples because it was the one I was using the most to test changes. I find it helpful in finding out the effect of a prop change and what is the effect of the various different combinations of props.

By changing the props interactively, the effects of the change are quickly shown as opposed to creating a new simple example with a particular combination of props. I did have to make several changes due to problems found when mixing props in different combinations.

You may want to put this example last behind the simpler examples to be the last one a user may visit.

jakezatecky · 2019-05-26T07:38:11Z

src/js/TreeNode.js

@@ -6,6 +7,8 @@ import Button from './Button';
 import NativeCheckbox from './NativeCheckbox';
 import iconsShape from './shapes/iconsShape';
 import languageShape from './shapes/languageShape';
+import nodeShape from './shapes/nodeShape';
+


Minor, but please remove this extra line.

jakezatecky · 2019-05-26T07:44:34Z

src/js/CheckboxTree.js

-    isSomeChildChecked(node) {
-        return node.children.some(child => this.state.model.getNode(child.value).checkState > 0);
+    isParent(node) {
+        return !!(node.children && node.children.length > 0);


This double negation seems unnecessary.

I think is is necessary if node.children is undefined. For example, if it is a leaf node. In that case you would be returning undefined instead of false.

jakezatecky · 2019-05-26T07:55:38Z

src/js/CheckboxTree.js

+            const isRadioGroup = !!node.radioGroup;
+            const isRadioNode = !!parent.radioGroup;
+
+            /*


If I am to understand this correctly, this is for when developers might have more than one of the radio options selected and that this would reduce them to one? I think it is fine deselecting all other radio options when a user would select a new item but if the developer had multiple radio options checked, then that is their own fault and I rather not fix that for them. This should not occur during normal user operations, correct? This would only occur if there was a developer issue.

I wasn't sure how to handle this. In a previous project, the code corrected radio group problems where no button was checked or too many.

The reason for this is that the program is generic and gets a map layer tree configuration from a user supplied json file. It is not the developer making the mistake but the user. Having a default for no check or too many checks seemed reasonable as a mistake made the map unusable.

I had it commented out as I was not sure I wanted it here. I think for my projects, I will have a separate config validator outside the main application.

jakezatecky · 2019-05-26T08:01:32Z

src/js/CheckboxTree.js

-            flatNode.checkState = this.determineShallowCheckState(node, noCascade);
+            // determine if node children need to be disabled
+            let disableChildren = false;
+            if ((!noCascade && nodeDisabled) || (isRadioNode && !node.checked)) {


This check for a parent node being checked before radio options may be selected could present an issue if the parent has showCheckbox: false, as the parent could never be selected. Otherwise I think it makes sense.

Thanks for pointing this out. You have caught a bug.

My thinking on this is that a radioGroup node, the parent of the 'radio button' nodes, should always have a checkbox. It works as a leaf in the tree to that point. Each radio button below it works as the start of another tree.

We probably need to work through some further discussion on this as from some of your comments I see you have a different use case using the checked array. I will start that discussion in a new comment at the bottom.

jakezatecky · 2019-05-26T08:04:43Z

src/js/CheckboxTree.js


+        const { icons, id } = this.state;
+
+        let state1counter = 0;


These statecounter variables are very scary. I admittedly have little idea of what is going on here but I am averse to seeing variables of the format var1 and var2. I would prefer more semantically named variables.

These are semantically named variables. :)

state1counter is counting the number of nodes which have checkstate === 1 in the current call of the recursive function renderTreeNodes. state2counter does the same for checkstate === 2.

I was thinking of using checkstate1counter or checkstateEquals1Counter but opted for the shorter versions. Would you prefer one of the longer names? If not, I should probably explain it in the comments.

As for your understanding what is going on, I probably need better comments.

Each call to renderTreeNodes is processing the children of a parent node. These counters are tabulating the number of children with 'full' or 'partial' checks, checkstate 1 and 2 respectively.

These counters are returned from renderTreeNodes as numFullcheck and numPartialcheck. These variables are then used in the calculation of the checkstate of the parent node. This parent node is in turn the child of another parent and thus tabulated by the counter for its parent.

Let me know if this does not make sense. It took me a while to figure out how to do this in an efficient fashion.

jakezatecky · 2019-05-26T08:09:01Z

src/js/CheckboxTree.js

+        this.parents = {};
+
+        let root;
+        if (Array.isArray(nodes)) {


I mentioned this elsewhere, but I do not really see the point of a root object. Is this used for some types of calculation? I would strongly prefer that such a root-level object need not exist.

Covered in previous discussion...

worthlutz · 2019-05-28T20:29:41Z

@jakezatecky
With regard to the value array, checked, previously used to hold state. This array could be passed back as an argument to the onCheck function. The problem is that you need to walk the entire tree to load the array as the checking of the node has made changes which may have cascaded. It seems too complicated to search the array to add and subtract the changed values. Walking the tree does not take too much time but I would not want to do it unless needed. A static function could be written to do this walk and generate the array and that may be the best path to follow.

static getCheckedArray = (nodes, checkmode) => {
   let checked = [];
   ...
   return checked;
}

Or maybe an instance function could be passed as an argument to the onCheck function which would 'know' the props passed to CheckboxTree. I need to think about this more...

This reverts commit 7695036.

method rendertreenodes is converted to method processNodes This allows calculation of the checked array in the onCheck method with the same logic as used when rendering. This can be controlled with a new prop, useCheckedArray.

Worth Lutz added 2 commits May 14, 2019 15:39

fix lint errors

7d16e88

Worth Lutz added 4 commits May 15, 2019 10:02

fix noCascade and disable problem

bd7cfdf

fix or temporarily disable tests to allow build

a49f9f0

fix scss and less to pass linter for build

3dba178

Revert "fix scss and less ..." -broke travis, worked locally

e4d7084

This reverts commit 3dba178.

jakezatecky added this to the v2.0.0 milestone May 24, 2019

jakezatecky requested changes May 26, 2019

View reviewed changes

Worth Lutz added 14 commits May 28, 2019 16:38

merge v1.6-dev

460af8c

minor changes from review

e41fab8

fix radio/onlyLeafNode problem, add comments

a5e7b4b

remove id from PropTypes to pass eslint in build

53f0876

fix disabled status for radio nodes

c6340fb

fix comments

7695036

Revert "fix comments - stuff included by mistake"

e108df5

This reverts commit 7695036.

fix comments

da8fcf2

first try

90e5125

remove duplicate logic

ab48b5d

method rendertreenodes is converted to method processNodes This allows calculation of the checked array in the onCheck method with the same logic as used when rendering. This can be controlled with a new prop, useCheckedArray.

add useCheckedArray prop and fix examples

4f6ed9f

Merge branch 'add_checkedArray' into v2.0-proposal

8deb130

remove console statements

1e146e7

remove useCheckedArray from BasicExampleObject

d3138f0

worthlutz mentioned this pull request Jun 6, 2019

add Radio Groups #99

Closed

jakezatecky closed this Dec 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor for v2 #147

refactor for v2 #147

worthlutz commented May 14, 2019

worthlutz commented May 14, 2019

jakezatecky commented May 23, 2019

worthlutz commented May 24, 2019

jakezatecky left a comment •

edited

Loading

jakezatecky May 26, 2019

worthlutz May 28, 2019

jakezatecky May 26, 2019

worthlutz May 28, 2019

jakezatecky May 26, 2019

worthlutz May 28, 2019

jakezatecky May 26, 2019

worthlutz May 28, 2019

jakezatecky May 26, 2019

worthlutz May 28, 2019

jakezatecky May 26, 2019

worthlutz May 28, 2019

jakezatecky May 26, 2019

worthlutz May 28, 2019

jakezatecky May 26, 2019

worthlutz May 28, 2019

jakezatecky May 26, 2019

worthlutz May 28, 2019

jakezatecky May 26, 2019

worthlutz May 28, 2019

worthlutz commented May 28, 2019 •

edited

Loading

refactor for v2 #147

refactor for v2 #147

Conversation

worthlutz commented May 14, 2019

worthlutz commented May 14, 2019

jakezatecky commented May 23, 2019

worthlutz commented May 24, 2019

jakezatecky left a comment • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

worthlutz commented May 28, 2019 • edited Loading

jakezatecky left a comment •

edited

Loading

worthlutz commented May 28, 2019 •

edited

Loading