reagent-project / reagent-forms

Bootstrap form components for Reagent
339 stars 78 forks source link

Handle external updates again #29

Closed aeriksson closed 6 years ago

aeriksson commented 9 years ago

This fixes the issue with #28 where focus is lost when editing input fields.

aeriksson commented 9 years ago

FWIW the issue would appear when an input was bound to an atom that was deref'd by something other than reagent-forms, i.e. the following would work:

(reagent/render-component
  [#(let [doc (atom {})]
      (fn []
        [:div
         [bind-fields [:input.form-control {:field :text :id :a}] doc]
         [bind-fields [:input.form-control {:field :text :id :a}] doc]]))]
  (.getElementById js/document "app"))

…while the following would not:

(reagent/render-component
  [#(let [doc (atom {})]
      (fn []
        [:div
         [bind-fields [:input.form-control {:field :text :id :a}] doc]
         (:a @doc)]))]
  (.getElementById js/document "app"))

The change in 50cd0ef seems to fix it. Not sure why this is the case, though.

yogthos commented 9 years ago

This will also be an issue for the other fields as well though. I think the problem is that whenever the atom is dereferenced it causes a repaint and that loses the focus for the widget.

yogthos commented 9 years ago

Also, note that some of the widgets need to keep internal state so they have to return functions.

aeriksson commented 9 years ago

Good point. The problem seems to be that "sub-components" are re-mounted when a deref'd atom is changed.

For instance, the following works fine:

(def a (atom ""))
(defn put! [x] (reset! a (-> x .-target .-value)))

(reagent/render-component
  [(fn []
     [:div
      [:input {:value @a :on-change put!}]
      [:input {:value @a :on-change put!}]])]
  (.getelementbyid js/document "app"))

As does the following:

(reagent/render-component
  [(fn []
     [:div
      [(fn [] [:input {:value @a :on-change put!}])]
      [(fn [] [:input {:value @a :on-change put!}])]])]
  (.getelementbyid js/document "app2"))

However, in the following example, any change to a will cause the outer component to update. This, in turn, causes the inner component to be re-mounted, and focus is lost:

(reagent/render-component
  [(fn []
     [:div
      [:input {:value @a :on-change put!}]
      [(fn [] [:input {:value @a :on-change put!}])]])]
  (.getElementById js/document "app3"))

It should be possible to get around this by modifying the update behavior of the outer component.

oborder commented 9 years ago

Hi,

I am writing this here instead as an issue, I hope this is not inappropriate..

I am trying to disable some fields depending on the doc state. Without this patch input disabled state is not in sync with the doc (it seems to pickup last state from doc).

I have changed bind for :input-field to include disabled property, and it seems to work fine, but am unsure if this is the right approach.

https://github.com/oborder/reagent-forms/commit/422c6798bc5a91bf8e15398797bf58c038a51567

yogthos commented 9 years ago

@oborder it looks like the approach outlined in this issue won't really work, and the approach of adding the disabled property seems reasonable to me. If you'd like to pr that I can cut a new release.

oborder commented 9 years ago

@yogthos I may be doing something wrong, but my patch is not working ok withouth this patch. It seems to always render disabled state from previous document state. For example if I load document which should have some field disabled after document which doesnt have it disabled, the field will still be enabled, and then on next change of document it will be disabled.. I am not really sure what is causing this, and how to fix it?

yogthos commented 9 years ago

@oborder this patch doesn't work because it doesn't allow for local state in components. I was just keeping it open as a reminder to find a workaround for the issue. I should probably just close it and create an issue instead to avoid confusion.