[bug] Migrating from 1.5.1 to 1.6.0 resulting in some weirdness

thehig commented 4 years ago

I'm having some difficulty in migrating from 1.5.1 to 1.6.0.

I have resolved two separate issues that I will outline for anyone else that might get caught out by breaking changes in this PR.

Number 1: https://github.com/jakezatecky/react-checkbox-tree/issues/13

This issue adds support for 'checkModel' with modes 'all' or 'leaf'.

The old way was 'leaf'.
The new default is 'all'.
Adding checkModel="leaf" to the component resolves this

Number 2: https://github.com/jakezatecky/react-checkbox-tree/issues/171

This issue adds support for empty folders.

The old way if a node had empty children, it would behave as though it were a leaf.
The new way, if a node has empty children, it behaves as though it is an empty folder.
Modifying the datastructure for the checkbox tree to 'null out' the children when we are adding a leaf

node = {
  value: node_name,
  label: name,
  children: isLeaf ? null : [] // If this is a leaf, it should not have a children array
};

But now I'm getting some weirdness, and this is where the 'bug report' aspect of things begins:

I have a CheckboxTree that contains a list of nested folders.
Some folders in the CheckboxTree are empty (empty children array)
The CheckboxTree has checkModel="leaf" set

As a result of this, when I have an empty folder nested somewhere in my structure and I initialize the component with an empty checked array, these empty folders are becoming checked somehow.

I will repeat for clarity. My checked array is empty, but the "empty-children" "non-leaf" nodes are defaulting to checked.

Screenshots

Default state, just after loading. checked array is empty

Same state as above, just "drilled down" to the offending elements

For clarity (since I have to remove the labels) the first and third nodes are expanded showing no children inside them. The second node also has no children, but is not expanded. The fourth node has children and is expanded. This is all with an empty checked array and checkModel="leaf"

thehig commented 4 years ago

Simple POC project

Changing from 1.6.0-alpha.2 to 1.6.0 in the package.json demonstrates the change in behavior

jakezatecky commented 4 years ago

Thank you for the report and the CodeSandbox project!

For Issue Number 1, the default is still leaf. I am not sure how it would be defaulting to "all" for you. Removing the checkModel property from your example does not change the logged values. Can you point to an example that shows checkModel defaulting to "all"?

For Issue Number 2, in hindsight I admit that this change from previous behavior probably warranted a semver major update. Unfortunately, I did not consider that when I accepted the PR because it made sense to treat any node with an empty children array as a folder and I did not imagine anyone would rely on empty arrays to be treated as leafs. I think setting it to null as your example is the appropriate way to do things, and I should probably include a reference to this in the CHANGELOG.

Per your last point, I did not realize that empty folders were getting checked. I looked at the PR's tests, and they seemed okay. I should have rendered an example in the browser to see how it panned out; that is a big mistake on my part and the current behavior is confusing to users.

The empty parent nodes only appear checked because of how the check algorithm works. I can add simple test in case they have no children, but that still leaves the checkbox open for interaction when logically there is nothing to really check (when checkModel="leaf", that is). I will have to think of how to deal with the checkbox for empty folders.

thehig commented 4 years ago

I suggest that allowing a user to check/uncheck an empty node is an acceptable behavior. Consider the case of a file-manager. Being able to 'check' an empty folder would be required for being able to mark an empty folder for copy/delete etc

plemarquand commented 4 years ago

Chiming in here to say that this should definitely be a semver major update. We optimistically create the children array when building our data structure expecting the node to behave as a leaf if the array is empty. Updating to 1.6.0 broke our build.

jakezatecky / react-checkbox-tree