Skip to content

add Radio Groups #99

New issue

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

By clicking “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? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Conversation

worthlutz
Copy link
Contributor

@worthlutz worthlutz commented Jul 23, 2018

This pull request adds Radio Groups to react-checkbox-tree. This allows mutually exclusive options with radio buttons to be used in the tree.

This includes the changes made in #97 and #98. I submitted #97 because some small refactoring was needed to make this work. If you can accept the refactoring in that pull request, then this one makes the same changes.

I have updated the README and the examples. I still need to add some tests.

@jakezatecky, let me know if you will accept this pull request. I need the radio group option and would prefer to not fork your project.

Let me know if you see any changes needed to keep consistent with your project.

@jakezatecky
Copy link
Owner

Thanks for the PR. I'll look into this on the weekend.

@jakezatecky
Copy link
Owner

Thanks for the PR! It is a logical extension of the current component, and I definitely agree that it would be a useful addition.

Disclaimer: I have scanned the file changes from this PR, but have not fully looked at all code changes.

From my testing of RadionExample.js, I have noticed a couple issues, one of which is minor and non-blocking:

  1. Parental radio group selections are outputted to the checked state. I understand why this is being done. This is in conflict with the original design of the tree, where only leaf nodes are recorded. There is a seperate issue, Checking all children of a node should only check the root node #13, that deals with two other models, one of which would include all checked nodes (model 3), a bit like how the radio groups currently function in this PR. Until we have full support for all three models detailed in Checking all children of a node should only check the root node #13, I would strongly prefer that we stick to the original model where only leaf nodes are reported.
    • I understand that this might conflict with the ability to preserve selections under a radio group when a parental radio group becomes deselected, which could result in a degredation of the user's experience, but I would strongly desire consistency between the checkboxes and radio groups, because
    • We could still keep record of the the child selections below an unselected radio node, but that might have to be stored differently than the checked state, which might be messy.
  2. (Minor) I think I would prefer that all children of a radio node are disabled unless that radio node is active. That should enhance user experience by not allowing users to set something that would ultimately not be recorded. I could probably add this later, though.

Honsetly, the issue with point 1 is probably a sympton of the fact that the checked array is just a flattened array of keys. Issue #13, while seemignly unrelated, is also currently difficult because we use an array of keys, which is convinient for form information, but less convient for state. This would all be easier if the nodes themselves contained state that would be updated. But that might be a change for another time.

I might have to spend a little more time thinking about this when I get some extra free time.

@worthlutz worthlutz mentioned this pull request Jul 30, 2018
@worthlutz
Copy link
Contributor Author

It looks like you have gotten a good idea of what some of the problems are. :)

I agree completely with point 2 and had planned to add that.

As for point 1, I agree. I have thought about much of those same concerns, even thinking about suggesting changes for a v2.0 replacing the checked array. In previous projects of mine, the state of the tree was contained in the nodes object. This worked well but looks like it might cause problems with React. I am a relatively new React user and still have problems with what causes re-rendering. Storing the tree state in the tree would require mutating from the changed node to the root, I think. It would also require every node to keep its own checked state. It seems possible but was more than I wanted to attempt at this point. I needed something to work now. :)

Requiring the radio buttons nodes to be leaves would simplify things and instead of using the radio group checkbox to turn on or off the group, one would be able to "uncheck" a checked radio button and thus have none selected that way. This should work just like the checkboxes with the only change being that sibling radio buttons get turned off when one is turned on. I think going this route may cause problems for expanding to what I have shown in the examples in the future.

After more thought, having the radio button nodes as leaves might work if then the child of that leaf could be its own CheckTree. Essentially that is how it is working now except that the checked array is polluted as you noted. Maybe this is the way to go? I'll think on this a bit.

What do you think?

@worthlutz worthlutz mentioned this pull request May 14, 2019
@worthlutz
Copy link
Contributor Author

I am closing this PR as it is outdated by changes to V1 since it was submitted and this capability is included in the V2.0 proposal #147.

@worthlutz worthlutz closed this Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants