day8 / re-com

A ClojureScript library of reusable components for Reagent
https://re-com.day8.com.au
MIT License
796 stars 147 forks source link

How does input-text handle keyboard events in the :on-change function? #187

Closed aeberts closed 5 years ago

aeberts commented 5 years ago

I'm attempting to port re-frame's todomvc to use re-com components and I'm confused by the way input-text handles keyboard events.

I'm using re-com/input-text to implement the text box that accepts a string from the user and creates a new todo item in the following code:

(defn new-todo-input
  [{:keys [on-save]}]
  (let [val (r/atom "")
        stop #(reset! val "")
        save #(let [v (-> @val str clojure.string/trim)]
                (on-save v)
                (stop))]
    (fn []
      [rc/box
       :size "auto"
       :child [rc/input-text
               :model val
               :placeholder "Enter a new todo"
               :on-change #(do (reset! val %) (save))
               :change-on-blur? true
               ]])))

;; How new-todo-input is called:

[new-todo-input
                             {:on-save #(dispatch [:add-todo %])}]

Hitting the enter key appears to fire the dispatch event OK, but I expected the input-text box to be cleared of text once the stop function is encountered but the text remains in the input-text until the escape key is typed.

What am I missing here? :-)

aeberts commented 5 years ago

@Gregg8 Many thanks for this update. I was struggling to figure out what I was doing wrong and I didn't have enough experience with re-com to debug it. Now I'm back on track! :-)

Gregg8 commented 5 years ago

You're welcome. It's been a bug forever and your issue was a good catalyst to fix it. This will be included in 2.2.0 which should be released shortly.

prook commented 3 years ago

Okay, I've done some digging today (see #219), and I think the example above is misleading, and the fix in ebad92b is not the right way to resolve it. Actually, that commit introduces a bug (dissected in detail in here) that allows the above example to work, but only by mistake.

Whats wrong with the example:

Initially, val ratom is set to "", and stays that way all the time. When on-change is called, it does the following:

  1. reset val to the value entered by the user
  2. dispatch a re-frame event that modifies some part of outside world the component does not subscribe to.
  3. reset val back to ""

From Reagent's perspective, this happens atomically, so no change is detected and no update is triggered. This, I think, is OK!

On the other hand, it is not ok for the input component to set its external-value to whatever "ASDF" the user put in (because no, the external value is "", not "ASDF"), trigger a refresh on itself to somewhat accidentally peek the real state of the outside world, which is "", and reset itself accordingly in another update. Again, the outside val atom has been "" all the time, it never changed. This is an awkward and accidental way to reset the component.

Wouldn't this be a more appropriate solution?

(defn new-todo-input
  [{:keys [on-save]}]
  (let [gen-id (fn [] (gensym "my-input"))
        id (reagent/atom (gen-id))                  ;; <<---- use a ratom to assign component's key
        save (fn [v]
               (on-save (str/trim v))
               (reset! id (gen-id)))]               ;; <<---- and update it each time we want to reset the component
    (fn [_]
      ^{:key @id}                                   ;; <<---- the update is triggered here
      [rc/box
       :size "auto"
       :child [rc/input-text
               :model ""
               :placeholder "Enter a new todo"
               :on-change save
               :change-on-blur? true]])))