reagent-project / reagent-forms

Bootstrap form components for Reagent
339 stars 78 forks source link

Weird radio-buttons behaviour, and element id's are duplicated #36

Closed antishok closed 9 years ago

antishok commented 9 years ago

I can't seem to get any bindings to work, following the examples. The text field isn't editable, the checkbox can't be toggled, the radio buttons are all wacky, and the doc isn't getting updated. I suppose it's some silly issue on my part.. but I can't figure it out. Using reagent 0.5.0-alpha, reagent-forms 0.4.1

Also, I don't understand why the elements should get an ID equal to the given :id path. They have nothing to do with each other. Also, for radio-buttons for example the :id is supposed to be the same for each radio, which causes multiple radios with the same ID, which isn't valid.

Thanks!

(ns streamz.core
  (:require [reagent.core :as r]
            [reagent-forms.core :as rf]))

(def form
  [:div
   [:input {:field :checkbox :id :checkboxselection}]
   [:input {:field :text :id :firstname}]
   [:input {:field :radio :value :a :name :foo :id :radioselection} "foo"]
   [:input {:field :radio :value :b :name :foo :id :radioselection} "bar"]
   [:input {:field :radio :value :c :name :foo :id :radioselection} "baz"]])

(defn app []
  (let [doc (atom {:radioselection :b 
                   :firstname ""
                   :checkboxselection true})]
    (fn []
      [:div
       [rf/bind-fields form doc]
       [:label (str @doc)]])))

(r/render [app] (.-body js/document))
yogthos commented 9 years ago

You need to use the Reagent atom by referencing it in your namespace [reagent.core :as reagent :refer [atom]] or doing r/atom.

The elements need to be linked up with the atom in some way, and the :id is used as way to uniquely identify each element. Since id is already unique I chose to use it. Otherwise you'd have to supply an additional identifier key. While technically id should be unique it doesn't affect the behavior in practice.

antishok commented 9 years ago

And indeed it was something silly, thank you :)

This fixes everything except for the radio buttons still being wacky - for example if you click the first one and then the second one, it won't check the second one.

As for the duplicate id's, it's problematic for example if using a label with a "for" attribute specifying the radio's id, for cases where you can't wrap the label around the input. And I still don't understand why the elements need to have an id at all...

yogthos commented 9 years ago

You need to specify where in the atom map the value of the element should be stored. For example, if you're saving first name then you might want to store it using a key called :first-name. The id is the name of the key in the document.

I'm not actually seeing the issue with the radio buttons that you're describing. When I click a radio button I see that value populated in the document each time.

I could switch to use name instead of id in case of radio buttons to address the last problem.

antishok commented 9 years ago

I understand that the :id key is needed in the input's properties map, it specifies where in the document is the value you want to bind to. What I don't understand is why the actual DOM elements end up getting this same :id in their ID attribute, e.g. <input type="radio" name="foo" id="radioselection" value="a" data-reactid=".0.0.2"> I see no reason for that, maybe it's just a byproduct of your decision to name the key as :id but then you should take care to not pass it to React, it shouldn't appear as id on the actual element.

And I can't switch to use name because the key is called :id and the :name is used for grouping, not for identifying each radio, so I don't understand that last suggestion either.

And about the wacky radios.. I don't know what to say, I'm just using that short code example and it happens. The doc is updated fine when I click them, but they are not getting checked properly. Should I link to a full repo?

yogthos commented 9 years ago

It's done intentionally for convenience. Since the id is generally unique I see no reason to have to specify one id for document binding and another id for the DOM. The id is a unique key for that element.

I've updated radio buttons to use :name for binding, so that should address that problem. All other elements have a single container, so using :id is correct in those scenarios.

If you can link to the repo, I'll take a look and see if I can reproduce the issue.

antishok commented 9 years ago

IMO the key should not be neither :name nor :id for any element type, especially not a different one for different element types. But, I can live with it :)

I'll link a repo tomorrow. Thank you!

yogthos commented 9 years ago

The original idea was to keep semantics as close as possible to regular HTML attributes. Since id and name would normally be used in this way I feel it's reasonable to use them for that purpose.

antishok commented 9 years ago

Here's a repo with the issue: https://github.com/antishok/reagent-forms-test (I just run lein cljsbuild once and open index.html in the browser) I click the 3rd checkbox and it gets checked fine, then click 2nd one and it doesn't get checked. Thanks

yogthos commented 9 years ago

When using the latest reagent-forms 0.4.2, the issue seems to be occurring with the 0.5.0-alpha release, but not with 0.4.3 stable one. The latest version now uses the :name key for radio buttons as discussed:

(def form
  [:div
   [:input {:field :radio :value :a :name :foo} "foo"]
   [:input {:field :radio :value :b :name :foo} "bar"]
   [:input {:field :radio :value :c :name :foo} "baz"]])
antishok commented 9 years ago

Ah yes it works with 0.4.3, thanks. I was sure I'd tried that. Don't know if this issue should be closed yet then, I'll leave it up to you

yogthos commented 9 years ago

I looked and the logic for radio buttons can actually be improved a bit, I'm going to push a new version out today that should work with both. I'll close the issue for now though.