reagent-project / reagent-forms

Bootstrap form components for Reagent
339 stars 78 forks source link

Properly handle external updates to atoms. #28

Closed aeriksson closed 9 years ago

aeriksson commented 9 years ago

Currently, some forms do not properly update when the underlying atom changes.

For instance, in the following example (which has two selects operating on the same atom), changing one select will not update the other.

(let [doc (atom {})
        form [:select.form-control {:field :list :id :derp}
              [:option {:key :foo} "foo"]
              [:option {:key :bar} "bar"]]]
    [:div
     [bind-fields form doc]
     [bind-fields form doc]])

Obviously, this behavior is not very reactive, and can cause headaches (it seems to be the problem in #19).

This commit fixes this behavior by making bind-fields return a form directly (rather than returning a function which returns a pre-computed form).

While this will incur a slight performance penalty on updates relative to the alternative, it shouldn't be noticeable.

yogthos commented 9 years ago

That seems reasonable, and I definitely agree the more intuitive behavior is worth the performance penalty.

yogthos commented 9 years ago

@aeriksson I was doing a bit of testing and this change appears to introduce an undesirable behavior where fields lose focus immediately. For example, if you have an input field and you type in a letter it will immediately lose focus due to repaint.

aeriksson commented 9 years ago

That's not good!

How can I reproduce that problem?

I tried playing with the following in Firefox 36 and Chrome 38, but it seems to work fine:

(let [doc (atom {})
      form [:input.form-control {:field :text :id :first-name}]]
  [:div
   [bind-fields form doc]
   [bind-fields form doc]])
yogthos commented 9 years ago

I used the forms-example from this project and for example if you start filing out the last name the focus gets lost. I'm on OS X here and tried Chrome/FF with the same result.

yogthos commented 9 years ago

And I just tried your example and it works as expected, not really sure why it breaks with the larger one.