web3ui / web3uikit

Lightweight reusable Web3 UI components for dapps.
https://web3uikit.com
MIT License
1.72k stars 269 forks source link

Checkbox component does not have controlled state #766

Open rayyan224 opened 2 years ago

rayyan224 commented 2 years ago

The checkbox component should have a controlled state i.e props should directly control what the view is showing. Here's an example where it doesn't, we want props and state to be tightly coupled in order to reduce memory leaks.

Screen Shot 2022-08-08 at 12 02 20 PM
surajgjadhav commented 1 year ago

Hi, I found the root cause of that issue and also observed checkbox is not behaving properly for "Box on by Default" story.

https://github.com/web3ui/web3uikit/assets/31582237/d8e64abc-ac0e-41e8-b4b8-701802692073

This issue was happening. because, their is state issue in component itself. We need to pass state variable to the checked prop instead of passing the argument variable. https://github.com/web3ui/web3uikit/blob/c100f43f425f98bf01215e64f4e5c7fe95f07cd4/packages/core/src/lib/Checkbox/Checkbox.tsx#L58 this should be:

checked={isChecked}

should align with state variable instead of normal argument and our checked argument variable is kind of initial value of checkbox. so, ideally we should remove it from control as we can't align it with state.

If this is accepted please assign this to me and I will raise PR for that.

Thanks

surajgjadhav commented 1 year ago

Hi @BillyG83 , Could you please confirm on this:

Hi, I found the root cause of that issue and also observed checkbox is not behaving properly for "Box on by Default" story.

web3uiCheckbox-issue.mp4 This issue was happening. because, their is state issue in component itself. We need to pass state variable to the checked prop instead of passing the argument variable.

https://github.com/web3ui/web3uikit/blob/c100f43f425f98bf01215e64f4e5c7fe95f07cd4/packages/core/src/lib/Checkbox/Checkbox.tsx#L58

this should be:

checked={isChecked}

should align with state variable instead of normal argument and our checked argument variable is kind of initial value of checkbox. so, ideally we should remove it from control as we can't align it with state.

If this is accepted please assign this to me and I will raise PR for that.

Thanks