search

reagent-project / reagent-forms

Bootstrap form components for Reagent
339 stars 78 forks source link

Binding Input Form to a key pair of a map from the reagent atom #49

Closed hellonico closed 9 years ago

hellonico commented 9 years ago

So, I have the atom defined as:

(def fields (reagent/atom {}))

and fields are set by an ajax call. Then in a template, I have the following.

(for [[k v] (@fields :props)]
    [:input.form-control 
   {:field :text 
    :id (get @fields k)
    }])

I can't figure out how to get the pieces together. How to bind the value of key,value pair from a map in the fields atom ?

yogthos commented 9 years ago

You should be able to do something like the following:

(def fields (reagent/atom {:props {:foo :prop1 :bar :prop2}}))

(defn form []
  [:div
   (for [[k v] (@fields :props)]
    [:input.form-control 
     {:field :text 
      :id v}])])

(defn home-page []
  (let [doc (atom {})]
    (fn []
      [:div
       [bind-fields (form) doc]       
       [:p (str @doc)]])))

The fields could be populated using an Ajax call, and then you would bind them to the form and use the form with a separate document atom.

hellonico commented 9 years ago

In the example above, how do you display the initial values, :foo and :bar, in the input field ? if I do:

(defn form []
 [:div
   (for [[k v] (@fields :props)]
   [:input.form-control 
   {:field :text 
    :value k
    :id v}])])

Then I cannot edit the value by typing in the input field.

screen shot 2015-04-02 at 9 46 01 am

yogthos commented 9 years ago

The values have to come from the doc, so simply populate that with the data.

hellonico commented 9 years ago

Thanks for all the help. Along the way, I realize some of the keys had spaces and things were not updating.

The below gives the summary:

(def fields (reagent/atom 
  {:props (clojure.walk/keywordize-keys {"prop 1" :foo  :prop2 :bar})}))

(defn form []
  [:div
   (for [[k v] (@fields :props)]
    [:input.form-control 
     {:field :text 
      :id (str k)}])])

(defn edit-page []
  (let [doc (atom (@fields :props))]
    (fn []
      [:div
       [bind-fields (form) doc]       
       [:p (str @doc)]])))
yogthos commented 9 years ago

That looks correct to me, sounds like we can close it. :)

ghost commented 8 years ago

I believe I have basically the same problem: I have a form that is defined through an atom that is filled after loading the document, by clicking the "Reload" button. However the <select> element stays empty, despite the @applist clearly containing items.

Could you please give me a hint on what is going wrong here? And maybe add an brief example for a use-case such as this?

(ns mwe.core
  (:require [reagent.core :as r]
            [reagent-forms.core :refer [bind-fields]]))

(enable-console-print!)

(defn appdata2option
  [{id :id name :name}]
  [:option {:key (keyword id) :value id} name])

(def applist (r/atom []))

(defn reload-applist []
  (reset! applist [{:name "Test #1" :id :test-1}]))

(defn form []
  [:div
   (println "Reloading with applist:" @applist)
   (println "And generated options:" (map appdata2option @applist))
   (into
     [:select {:field :list
               :id :appid}]
     (map appdata2option @applist))
   [:input {:id :reload
            :type :button
            :value "Reload"
            :on-click #(reload-applist)}]])

(defn reagent-app []
  (let [doc (r/atom {})]
    (fn []
     [:div
      [bind-fields (form) doc]
      [:label (str @doc)]
      [:label (str @applist)]])))

(r/render-component [reagent-app]
  (.getElementById js/document "app"))

The associated HTML file looks like this:

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">

    <link rel="stylesheet" type="text/css" href="css/reagent-forms.css">

    <link rel="stylesheet" type="text/css" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/css/bootstrap.min.css">
    <link rel="stylesheet" type="text/css" href="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/css/bootstrap-theme.min.css">

    <title>MWE</title>
  </head>
  <body>
    <div id="app"></div>

    <script type="application/javascript" src="http://code.jquery.com/jquery-2.2.1.min.js"></script>
    <script type="application/javascript" src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.6/js/bootstrap.min.js"></script>

    <script type="text/javascript" src="cljs/mwe.js"></script>
  </body>
</html>

I'm omitting project.clj for brevity.

yogthos commented 8 years ago

Seems like the problem is that you're binding the form to doc, while you're reloading the applist.

yogthos commented 8 years ago

However, another problem is that the from is evaluated once and its output is passed to bind-fields. So, the list inside the select can't be modified the way you're trying to do.

ghost commented 8 years ago

Seems like the problem is that you're binding the form to doc, while you're reloading the applist.

Unless I misunderstood you: That is intentional. I load applist from a server and need to populate a [:select] field with [:options] based on that list.

Loading the applist might take a while and I want to start to show the form as quickly as possible. Thus I do not want to delay r/render-component until loading has finished.

However, another problem is that the from is evaluated once and its output is passed to bind-fields. So, the list inside the select can't be modified the way you're trying to do.

As I understand Reagent, the anonymous (fn) in reagent-app should be called whenever a referenced r/atom changes. Thus also when applist changes. Since the second parameter to bind-fields changed, Reagent should also re-evaluate that component, shouldn't it?

How would I do this correctly? Is there a way that allows me to "rebind-fields"? I.e. re-evaluate (form) and bind the result to doc?

The code I wrote IMO looks very similar to what you proposed earlier: https://github.com/reagent-project/reagent-forms/issues/49#issuecomment-88474457 I don't understand why your code works and mine does not. (I never actually tested yours, though.)

yogthos commented 8 years ago

The anoymous fn would change, but I don't think anything triggers [bind-fields (form) doc] to update. The doc would have to change for that.

ghost commented 8 years ago

I did as you suggested and store the applist inside the doc now, so that it will update and hopefully trigger rebuilding the [bind-fields] component:

(ns mwe.core
  (:require [reagent.core :as r]
            [reagent-forms.core :refer [bind-fields]]))

(enable-console-print!)

(defn appdata2option
  [{id :id name :name}]
  [:option {:key (keyword id) :value id} name])

(def doc (r/atom {}))

(defn reload-applist []
  (swap! doc assoc :applist [{:name "Test #1" :id :test-1}]))

(defn form [doc]
  [:div
   (println "Reloading with applist:" (:applist @doc))
   (println "And generated options:" (map appdata2option (:applist @doc)))
   [:select {:field :list
             :id :appid}
            (map appdata2option (:applist @doc))]
   [:input {:id :reload
            :type :button
            :value "Reload"
            :on-click #(reload-applist)}]])

(defn reagent-app []
 [:div
  [bind-fields (form doc) doc]
  [:label (str @doc)]])

(r/render-component [reagent-app]
  (.getElementById js/document "app"))

However, the issue stays: (form doc) is being called, it sees the correct (:applist @doc), but the HTML is not updated.

I assume I am severely misunderstanding something?

yogthos commented 8 years ago

Ah right, I think the problem is here. Since bind-fields returns a fn, that's what gets called on subsequent runs. So, the new template never gets passed in. So I don't think dynamically recreating the form would be possible.

ghost commented 8 years ago

Would it be possible to change Reagent Forms to support this use-case?

yogthos commented 8 years ago

Unfortunately, there isn't an easy fix for this. If I think of something, I'll let your know.

ghost commented 8 years ago

a4f316fdfe8967fc3ef1eef5ebad3b5cb7586320 contains the easy fix for this. It simply changes bind-fields to return form instead of (fn [] form), which, as a side-effect, also causes the form to be examined by reagent-forms every time the atom changes. It was submitted as PR #98.

If you want a more complex fix, which retains the behaviour of Reagent Forms to only examine the form once, but still allow it to change (in a non-structural way, e.g. by adding [:option]s) later, please tell me, so I can revise this patch.

yogthos commented 8 years ago

As I recall returning the form instead of fn would also break some of the fields as well. It's definitely problematic from performance perspective I think, so a more comprehensive solution would be needed. If you come up with anything, I'd be happy to merge that in and help with the testing.

ghost commented 8 years ago

As I recall returning the form instead of fn would also break some of the fields as well.

Could you please elaborate on that? Could you tell me, which fields I should specifically test?

It's definitely problematic from performance perspective I think, so a more comprehensive solution would be needed.

Could you please point me to some reputable documents on how to optimise code for Reagent, which goes more in-depth on this matter? Maybe you also know a way to properly benchmark this? Before I start with random optimisations, I would like to know which changes would likely yield a reasonable gain and which are just premature.

Please excuse me asking for so much hand-holding, but I am quite new to both ClojureScript and Reagent, as well as web-application programming in general.

yogthos commented 8 years ago

It's been a little while, I'll have to take a look at what specifically broke last time I tried this.

I'm not aware of any Reagent specific documentation on optimization. A lot of the standard React documentation applies however. I've also used the Chrome profiler to see what ends up being called and how often.

In general, you would want to ensure that components aren't repainted unless the data they're bound to has changed. So, as a rule of thumb is to minimize recalculation to the specific nodes that need it. Rerunning the whole bind-forms function each time a piece of data changes in the document would scale very poorly.