Skip to content

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

New issue

Have a question about this project? No Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “No Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? No Sign in to your account

Open
thehig opened this issue Jan 21, 2020 · 4 comments
Open

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

thehig opened this issue Jan 21, 2020 · 4 comments
Labels
Milestone

Comments

@thehig
Copy link

thehig commented Jan 21, 2020

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: #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: #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
image

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

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
Copy link
Author

thehig commented Jan 21, 2020

Simple POC project

Edit react-checkbox-tree example

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

@jakezatecky
Copy link
Owner

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.

@jakezatecky jakezatecky added this to the v1.7.0 milestone Jan 22, 2020
@thehig
Copy link
Author

thehig commented Jan 22, 2020

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
Copy link

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 jakezatecky modified the milestones: v1.7.0, v1.8.0 Jun 8, 2021
No Sign up for free to join this conversation on GitHub. Already have an account? No Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants