-
Notifications
You must be signed in to change notification settings - Fork 317
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
Overcome a limitation of binding data variables to checkboxes #214
Overcome a limitation of binding data variables to checkboxes #214
Conversation
Thanks for all your contributions, happy to have them! I'll need to take a closer look at this one. This week is quite busy so I'll get back to you after that. |
5b775c1
to
18d91fe
Compare
Hi, it took me a while, but I've finally taken a closer look at this. I think the initial issue you report can simply be resolved by returning eg. the value "on" for checkboxes that have no specified value, this is also what web browsers seem to do. Overall, I agree that the
What do you say we instead try out my suggested fix above? If we still want to change the checked controller we can discuss that, but it needs to happen in a non-breaking way and I need confidence that it works for both checkboxes and radio buttons. |
It's my sense of order that compelled me to come up with a solution that I considered cleaner from a certain point of view. With that being said, I could totally image that this change would also imply that other parts of the code would require added complexity so as not to introduce regressions.
If it's this part of the patch that you're referring to, then I intended it to be a form of simplification, which I now see that it isn't - I seem to have missed the
My take on this is that I'd be perfectly happy with any solution that you considered acceptable. Other than that, I'd also be absolutely willing to make improvements to this patch if you saw merit in its fundamentals. Since it's completely understandable that you'd like to avoid breaking changes, do you think that perhaps we could always emit |
I'm all for seeing some improved code, so please go ahead, I'm just happy you are motivated for it! :) Sounds good to me to add Let's try to make sure any new code doesn't break anything, and also works well with radio buttons. It would also be helpful if unrelated cleanups are kept in a separate commit, or even separated out to another PR. This way it is easier to following along the featured changes, thanks! |
18d91fe
to
f91d354
Compare
I went ahead and incorporated everything that has come up so far. EDIT: I've come across the |
f91d354
to
23be431
Compare
23be431
to
0f0dc9b
Compare
I've tested the changes using the databinding sample application and came up with something that is both generic and works for radio buttons. I personally believe that the Looking forward to hearing your feedback! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, now we are getting close! This is already an improvement. And I don't see any reason this would break anything, which is good!
I have some feedback below, let me know what you think.
I tested the latest changes using the databinding sample and things still work as expected. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, all of this looks good to me!
It's always nice when we can delete more lines than we add :)
I'm glad you like it! It's been a pleasure. Would you please consider making a release out of the bug fixes that have accumulated since the latest release? |
Sure, I'll do it soon! |
When binding data variables to checkboxes I encountered that the presence of a non-empty
value
attribute is required in order for them to properly function. I consider this a limitation, so I devised a solution to overcome it. I believe that the resulting changes make the underlying philosophy more streamlined.In my opinion, channeling the changes of the
checked
attribute of checkboxes into imaginary changes invalue
and then blending them together with radio buttons confuse things quite a bit, hence my attempt at cleaning it up.Any comments or thoughts are much appreciated!