Open MrJman006 opened 1 year ago
I don't mind drafting the changes to the documentation, but wanted to verify what the expected functionality is first before submitting a pull request for code or documentation changes.
Hi, i dont use tinybind anymore due to such limitations
I can grant access to repository if you want
@blikblum definitely interested in keeping this project alive as a "micro framework" . there is some usefulness still to be had here and will try to help starting with @MrJman006 patch if he is willing to help.
Im interested in keeping it alive also. Im up for getting access to the repo. In terms of documentation on the website how do you manage that?
Im interested in keeping it alive also. Im up for getting access to the repo. In terms of documentation on the website how do you manage that?
It seems he has a gh-pages branch where it appears to have the site and the documentation.
I do not see a major overhaul at the moment once taking over the project and with it being a micro framework we can continue with the current site/ documentation used with the necessary edits and updates.
My only suggestion might be toi move it to its own repo to separate from the core project. We can maybe use the repo Wiki for more detailed documenting( like custom components and adapters) as the code would be near by. Still up for debate though
Issue Discussion
I've been using Tinybind for a small personal project and came across a documentation bug for the
checked
binder. The binder documentation states the following.Checked Checks the input when the value evaluates to true and unchecks the input when the value evaluates to false. This also sets the bound object's value to true/false when the user checks/unchecks the input (two-way). Use this instead of value when binding to checkboxes or radio buttons.
Checkbox inputs using the
checked
binder work as expected, but radio inputs do not. Below is a description of the actual behavior and a minimal demo I put together.Based on the documentation, I put together a data structure to bind to some radio inputs.
var DATA = { "radios": [ { // Binds to the radio ID attribute. "id": "d1r1", // Binds to the radio value attribute. "value": "radio-1", // Used for the label associated with the radio. "label": "Radio 1", // Binds to the radio checked DOM value. "checked": true, // Helper variable to display the actual DOM value associated // with the radio. "_value": undefined, // Helper variable to display the actual DOM checked state // associated with the radio. "_checked": undefined }, { "id": "d1r2", "value": "radio-2", "label": "Radio 2", "checked": false, "_value": undefined, "_checked": undefined }, { "id": "d1r3", "value": "radio-3", "label": "Radio 3", "checked": false, "_value": undefined, "_checked": undefined } ] };
One thing that I'm curious about was
Why use
rv-checked
and notrv-value
for radios? While check boxes and radio buttons act similar their business logic is quite different.
Check Boxes are more of an optionally "enabled" "disabled" logic while radio buttons are more of a required "choose either or" value based logic. They should be treated different even though they have a similar functionality with Radio buttons using onclick
event listeners instead with a "select option" apprach as you suggested, as radio buttons are always plural and not singular.
Perhaps we can mandate that the template syntax be such that the markup provides a grouping wit the nested radio buttons and a parent model as you suggested.
I dont't see why we need to make the recommended patch though has his logic has merit we simply need to "add specifity".
I'm yet to run tests for this issue but will look at it as I may need this functionality especially, along with this issue
I added both as collaborators to this repo.
The docs are in gh-pages branch, _harp folder
While check boxes and radio buttons act similar their business logic is quite different. Check Boxes are more of an optionally "enabled" "disabled" logic while radio buttons are more of a required "choose either or" value based logic. They should be treated different even though they have a similar functionality...
I completely agree. Checkbox inputs and radio inputs both have a checked state, but their usage is for boolean selection and set selection respectively.
Why use rv-checked and not rv-value for radios?
The value
binder can't be used because we would be breaking its semantics. There are several HTML elements that use the value
attribute. When using the value
binder with input elements specifically (excluding radio inputs) the semantic is a two way data binding between a model datum and the value
attribute. Changing this semantic for radio inputs not only breaks Tinybind users expectations when using the value
binder, but also prohibits the use of the each-*
binder as there would no longer be any way the user to set the value
attribute for each-*
generated elements.
Perhaps we can mandate that the template syntax be such that the markup provides a grouping wit the nested radio buttons and a parent model as you suggested.
So I'm torn on this point. I agree that we need some way to get different groups of radio inputs for the purposes of keeping the model data in sync, but I really dislike forcing the user to structure their page a specific way to make a Tinybind feature work. Especially when alternative page formats are valid HTML. Interestingly the HTML spec for radio inputs states that all radio inputs that are not explicitly grouped are to be treated as in the same group based on several structure rules. We might be able to use those structure rules to keep a group list at bind time and use that list to update the model data when changes occur. The problem is catching all of the situations where a radio input can have its checkedness status change some of we may not have any way of determining.
I dont't see why we need to make the recommended patch though has his logic has merit we simply need to "add specifity".
So some kind of patch is needed to fix the checked
binder for radio inputs because the semantics of the checked
binder for radio inputs are not satisfied currently. The patch in my original post, is only a partial fix. The rest of the fix requires changes to address the grouping issue discussed above since radio inputs are dependent on each other unlike all other input elements.
I also think having a new binder to handle the selected
functionality that I demo in my last GIF is important to have as we both agree that is a primary use case for radio buttons. I might move that to a separate issue to keep things organized.
My only suggestion might be toi move it to its own repo to separate from the core project.
Now that I see the documentation is on a branch of the project I think I understand that the website documentation is being pulled directly from that branch using GitHub pages. I guess we could technically put it in a separate repo if there is a compelling reason to do that, but I'm fine leaving it here as a branch also.
The docs are build with http://harpjs.com/docs/
@MrJman006 @blikblum
So I ran some tests and made this patch.
The premise is that we need to treat rv-value
for input[type=radio|checkbox] as a regular attribute, as the rv-checked
will do the data binding for us. Adding to this, while input[type=checkbox] needed only to be checked/unchecked input[type=radio] needs to select a variable's option value ( much like <select>
) thus we need to set rv-checked=variable|property
to be used instead of rv-checked=true|false
I added some new specs to test out the patch but not quite sure my tests were sound so if you guys want to give it go...have at it!
I also setup a demo wit the patch to show how we should be using radio buttons and checkboxes with both rv-checked
and rv-value
and will update the docs according once you guys review the modifications!
@blikblum one question though as you are more familiar with the project
what does this do and could we have used this to toggle event listening and publish() based on element type?
@MrJman006 @blikblum
So I ran some tests and made this patch.
The premise is that we need to treat
rv-value
for input[type=radio|checkbox] as a regular attribute, as therv-checked
will do the data binding for us. Adding to this, while input[type=checkbox] needed only to be checked/unchecked input[type=radio] needs to select a variable's option value ( much like<select>
) thus we need to setrv-checked=variable|property
to be used instead ofrv-checked=true|false
I added some new specs to test out the patch but not quite sure my tests were sound so if you guys want to give it go...have at it!
I also setup a demo wit the patch to show how we should be using radio buttons and checkboxes with both
rv-checked
andrv-value
and will update the docs according once you guys review the modifications!
I'm confused what the patch you added does? The checked binder already was setting a non-binary value for input[type=radio]
and the value
binder worked for both input[type=checkbox]
and input[type=radio]
. Maybe we need to chat together so we don't run over each other because I have been working on a patch as well to fix the checked
binder to handle boolean values as the documentation states and to add a new binder called select
that implements the current checked
functionality for radio inputs.
@blikblum one question though as you are more familiar with the project
what does this do and could we have used this to toggle event listening and publish() based on element type?
It is a state variable telling Tinybind which binders publish view changes back to the model. See here (src/view.js
lines 165-172.
@MrJman006 based on what I understood about your original issue, using the rv-checked
on radio buttons did not update the dom element accordingly. Your patch did fix that but I encountered another issue with it using it on checked boxes at it updated the text content to "true" instead of the value of the check box. There was another issue as well using both rv-checked
and rv-value
on the same element for checkboxes/ radio buttons as well with the patch. So i had to refactor the code a bit to fix both issues so that
rv-value
and rv-checked
What is needed to be done is to update the documentaion on showing the proper way to use the elements with the respective binders as necessary. I think this is the main part as it may not be properly conveyed with a distinction between the two elements.
Issue Discussion
I've been using Tinybind for a small personal project and came across a documentation bug for the
checked
binder. The binder documentation states the following.Checkbox inputs using the
checked
binder work as expected, but radio inputs do not. Below is a description of the actual behavior and a minimal demo I put together.Based on the documentation, I put together a data structure to bind to some radio inputs.
Here is the template code. The
updateLabels
function purely updates the label so I have omitted it here.The first issue that arises is a failure to check radio input when the associated model datum is set to the boolean
true
. In the demo below,Radio 1
should be checked because the associated model datum istrue
, but you can see it is not. Even setting the model datum explicitly after thebind
call does not update the DOM state for the radio inputs.The second issue is an incorrect updating of the model datum when the DOM state is changed. When a user clicks any of the radio inputs, the model datum associated with that radio input is not set to
true
as stated in the documentation. It is instead set to the DOM value associated with the radio input. If no value is set, the value'on'
is used.These bugs manifest with various configurations including inside a
<fieldset>
and even in the absence of thename
andvalue
attributes.I initially attempted to generate a patch that honors the intent of the
checked
binder based on the documentation, but another issue arises. Radio inputs do not emit any DOM events when the are unchecked. This means there is no hook for the library to update the underlying model datum for a radio input that becomes unchecked.I don't fully understand the rational for the HTML standard to exclude radios from emitting events when unchecked, but that is separate conversation.
What does work is treating radio inputs the same way you would treat a
select
input. Another way to say this is treating thechecked
binding as a string result across a group of radio input instead of a boolean result on a single radio input.New data structure.
New template code.
If this is the expected functionality, then the documentation needs to be updated to discuss how to properly use the
checked
binder with radio inputs.Demo Code
Patch Code