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

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

Closed
aeberts opened this issue Oct 7, 2018 · 3 comments
Closed

Comments

@aeberts
Copy link

aeberts commented Oct 7, 2018

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? :-)

@Gregg8 Gregg8 closed this as completed in ebad92b Nov 2, 2018
@aeberts
Copy link
Author

aeberts commented Nov 3, 2018

@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
Copy link
Contributor

Gregg8 commented Nov 4, 2018

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
Copy link

prook commented Nov 18, 2020

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]])))

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

No branches or pull requests

3 participants