Skip to content
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

Select input doesn't repaint selected value when state is changed externally #19

Open
erikcw opened this issue Nov 7, 2014 · 21 comments

Comments

@erikcw
Copy link

erikcw commented Nov 7, 2014

The :select list control doesn't display the correct value when initiated with state.

For example, if I pass `(atom {:status "P"}) into this form:

   (row "Status" [:select.form-control {:field :list :id :status}
                  [:option {:key "A"} "Active"]
                  [:option {:key "P"} "Paused"]
                  [:option {:key "X"} "Archived"]])

NOTE: I've tried both strings and keywords for the value of the :key attribute.

The form will render with the first option displayed ("Active"). The atom will continue to hold the value of "P" until the user selects a new value from the select.

The issue is also illustrated on the examples page where the select should display bar but instead displays foo.

I tested this in both the latest releases of Chrome and Firefox on OSX.

@yogthos
Copy link
Member

yogthos commented Nov 8, 2014

Ah looks like the bug was caused by trying to set the default selection to the value instead of the label in the control. I just pushed out 0.2.5 with a fix.

@erikcw
Copy link
Author

erikcw commented Nov 10, 2014

I just gave 0.2.5 a try and the issue seems to persist. The demo also is still exhibiting the issue.

@yogthos
Copy link
Member

yogthos commented Nov 10, 2014

I'm not able to reproduce the problem with 0.2.5, here's a sample project I've created and it appears to work as expected. I haven't updated the demo yet, thanks for pointing that out.

@erikcw
Copy link
Author

erikcw commented Nov 11, 2014

My mistake. I had 0.2.4 installed. lein clean....

@erikcw erikcw closed this as completed Nov 11, 2014
@erikcw
Copy link
Author

erikcw commented Nov 21, 2014

Looks like I found another corner case with this issue.

If you update the atom after the form has been bound with bind-fields and rendered, the select input will not update with the new value. I forked your demo and created a new branch for the issue. See the Pause/Activate/Archive buttons at the top of the form.

Interestingly enough, nothing happens after the first button click -- you have to click it a second time for the state to change. Perhaps it is "fighting" with the bound select field?

@erikcw erikcw reopened this Nov 21, 2014
@yogthos
Copy link
Member

yogthos commented Nov 21, 2014

The atom is meant to be managed by the bind-fields and updating it externally is going to cause undefined behavior. You should be using events to change the state of the atom instead. This is inherent in the design of the library.

@erikcw
Copy link
Author

erikcw commented Nov 21, 2014

Ok, so if I instead I use an event attached to bind-fields -- I get the same result, the select field doesn't update. I've pushed a new commit to my demo with an example.

@yogthos
Copy link
Member

yogthos commented Nov 21, 2014

Looks like you have to explicitly set selected on the option when the document value is changed, and the library currently doesn't support this behavior. I'll take a look to see about adding that.

@erikcw
Copy link
Author

erikcw commented Jun 18, 2015

Just wanted to note that wrapping bind-fields in an anonymous function and vector seemed to make this work. However every time any of the form fields are updated, it makes the form lose focus on keyup.

It looks like it just re-binds and then re-renders the whole form every time the atom changes. Nice that it works for external changes -- but it breaks everything else!

;; Causes form to re-render for both external (good) and internal (bad) changes
[(fn []
  [bind-fields my-form doc])]

@yogthos
Copy link
Member

yogthos commented Jun 18, 2015

yeah there are definitely a few quirks with this, not sure what the right way to fix it would be

@arathunku
Copy link

Hello,

is there any progress on that or someone could provide some initial help so I could work on a PR to fix this?

I've following form:

(ns survey.components.surveys.edit
  (:require [re-frame.core :refer [subscribe dispatch]]
            [taoensso.timbre :refer-macros (log spy)]
            [reagent-forms.core :refer [bind-fields]]))

(defn- row [label input]
  [:div.row
   [:div.col-md-2 [:label label]]
   [:div.col-md-5 input]])

(defn- add-question [s]
  (reset! s (update-in @s [:questions] assoc (survey.utils/unique-id) {:value ""})))

(defn- to-path [& keys]
  (spy :debug
    (str
      (first keys) "."
      (clojure.string/join "."
        (rest (map #(if (keyword? %) (name %) %) keys))))))

(defn- form [id]
  (let [s (subscribe [:find-survey id])
         survey (reagent.core/atom @s)]
    (fn []
      [:div ^{:key id} (str "Display: " id " with: " (-> @survey :questions keys count) " questions")
       [:form
        [bind-fields (row "name" [:input {:field :text :id :name}]) survey]
        [:div (for [k (keys (:questions @survey))]
                ^{:key k}
                [bind-fields (row (str "question " k) [:input {:fields :text :id (to-path :questions k :value)}]) survey])]
        [:button.btn.btn-primary {:on-click #(dispatch [:save-survey @survey])} "Save"]
        [:button.btn.btn-primary {:on-click #(add-question survey)} "Add question (local component state)"]]])))

(defn render [id]
  [(with-meta
     (partial form id)
     {:component-will-unmount #(dispatch [:destroy-survey id])})])

and when I call add-question, component resets current inputs and doesn't bind into fields from the loop 😢

One more question, is there any particular reason why the :id key in the input options doesn't accept sequence?

@yogthos
Copy link
Member

yogthos commented Aug 24, 2015

The :id can be composed of :foo.bar.baz that gets translated into a path [:foo :bar :baz]. The select issues still needs some investigation, but I simply haven't had the time. One option could be to use Bootstrap style dropdowns instead and avoid the whole problem that way.

@yogthos yogthos changed the title Select input doesn't display selected value from state Select input doesn't repaint selected value when state is changed externally Sep 4, 2015
@tawus
Copy link

tawus commented Jun 6, 2016

I am facing the same issue. I was looking at the code and it seems that :list uses the option text as value and if I set :value attribute in :options to the option text, it works for me.

@khdegraaf
Copy link

I can fix need for :value to be text (although replacing it with need for :value to be same as :key) and fix external updates with the code at the following:

https://gist.github.com/khdegraaf/55b8464b9b007500cf5dcd50de40d21f

@curtosis
Copy link

curtosis commented Dec 7, 2016

This seems to be related to the problem I'm seeing with a "dynamic" form-template argument to bind-fields. I'm generating the template with a function, so the template is static at bind-fields run time, but the fields don't re-render. Minimal case: https://gist.github.com/curtosis/a4c47dae45a81030d011cce550cb0e4a

@yogthos
Copy link
Member

yogthos commented Dec 7, 2016

Yeah, the current approach is to use events to modify the atom as seen here.

@curtosis
Copy link

curtosis commented Dec 7, 2016

I've been toying with the events, thinking they might help, but I don't think I've grokked how they would work. If I add an event handler to bind-fields it runs when I type in one of the text input fields, but clicking the button to inc the :user-count in the atom doesn't trigger it. Which kind of feels like being back at the same problem. (Emphasizing here that I don't understand it well enough yet.)

@yogthos
Copy link
Member

yogthos commented Dec 8, 2016

The forms aren't really meant to be used with dynamic content. The way you'd implement your example would be as follows:

(defn row [label input]
  [:div.row
   [:div.col-md-2 [:label label]]
   [:div.col-md-5 input]])

(defn test-form [doc]
  [:div
   [bind-fields
    [:button {:on-click #(swap! doc update :users conj {:username ""})} "Add User"]
    doc]
   [:div
    (for [idx (range (count (:users @doc)))]
      ^{:key idx}
      [bind-fields
       [:div [:span (str "User #" (inc idx) ":")]
        (row "user name" [:input {:field :text :id :username}])]
       (reagent/cursor doc [:users idx])])]
   [:label (str @doc)]])

(defn home-page []
  (let [doc (reagent/atom {:users []})]
    [:div [:h2 "Welcome to Reagent"]
     [test-form doc]]))

The layout can be managed outside bind-fields and you can call bind-fields on cursors into the original atom for each dynamic element.

@curtosis
Copy link

curtosis commented Dec 8, 2016

Ah, I see! I'd forgotten about cursors (spent some time in Om-land). I'll give that a shot. Thanks!

@efraimmgon
Copy link
Contributor

efraimmgon commented Sep 22, 2018

Shouldn't using it with re-frame solve the problem?

"(...) reagent-forms will not hold any internal state and functions provided by you will be used to get, save, and update the field's value".

@madstap
Copy link
Contributor

madstap commented Sep 22, 2018

@efraimmgon Not sure about that, I had to do something like this to get dynamic fields to work like I wanted. Which is, I think, basically the same as yogthos' approach above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants