day8 / re-com

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

Typeahead problems #165

Open lkonstantinov opened 6 years ago

lkonstantinov commented 6 years ago

Sorry if this reads as a disjointed rant - its not intended to be. Easiest thing for me is to do a PR with the "fixes" that seem right to me, but this is not my component, neither my library, so I would like to align my expectations with what the authors intended.

While the typeahead component looks fine in the demo, using it while wired to a model that is being updated makes me feel like a pig wrestling with a pumpkin :)

Consider the following oversimplified snippet:

...
metric (rf/subscribe [:metric])
...
[typeahead
                               :model metric
                               :data-source metric-datasource
                               :placeholder "Enter metric"
                               :change-on-blur? false
                               :on-change  (rf/dispatch-sync [:metric v]) ]

Pretty standard affair - I have the component wired to a model and when the data changes - the model gets updated.

A side comment - :change-on-blur? is broken if you want to navigate with the mouse - the component only tracks the TAB key as a method of losing the focus, so for users that like to click the :on-change event never seems to be fired. So for my intents and purposes, I am going to set it to false.

The first problem that I've hit is with this piece of code which gets executed every time the component needs to be redrawn (for example, when the data source invokes the callback with a list of suggestions):

(when (not= latest-external-model external-model)
          (swap! state-atom external-model-changed latest-external-model))

which invokes this function:

(defn- external-model-changed
  "Update state when the external model value has changed."
  [state new-value]
  (-> state
      ;(assoc :external-model new-value) ;; this is missing in the implementation
      (update-model new-value)
      (display-suggestion new-value)
      clear-suggestions))

the function never updates the value for :external-model, so in effect every time the component is redrawn, its state wrt suggestions is reset. What happens visually, is that the completion list shows up then is immediately closed. The flow of events is like this:

  1. user presses key
  2. input-text-on-change! gets invoked 2.1. the key press is put on a channel that is going to trigger the data source (this smells as well, but more on it in a bit) 2.2. the :model gets updated with the value
  3. component is redrawn and the autocompletion data gets reset (its fine for now, cause the data source havent kicked in, see 2.1. above)
  4. data source invokes the callback, the state atom changes
  5. component gets redrawn again, autocompletion data gets reset and the list is closed :(

With the fix above, on point 5 the autocompletion data wont get reset cause the :external-model and :model will match.

Note about point 2.1. - here is the input-text-on-change! code:

(defn- input-text-on-change!
  "Update state in response to `input-text` `on-change`, and put text on the `c-input` channel"
  [state-atom new-text]
  (let [{:as state :keys [input-text c-input]} @state-atom]
    (if (= new-text input-text) state ;; keypresses that do not change the value still call on-change, ignore these
        (do
          (when-not (clojure.string/blank? new-text) (put! c-input new-text))
          (swap! state-atom
                 #(cond-> %
                    :always (assoc :input-text new-text :displaying-suggestion? false)
                    (event-updates-model? state :input-text-changed) (update-model new-text)))))))

The channel value is pushed first and then the state gets updated. This has the theoretical potential of the data source kicking in first, then the on-change logic which can reorder the events and wipe the autocompletion state. I think the code needs to be reordered, but not sure if that will still be a guarantee, since I am not that familiar with the internal Reagent dispatching model.

So with this fix in place, the completion list stays on screen, but the moment you try to point your mouse at it or press an arrow to navigate, it performs a selection and closes down without any clicking involved.

What happens is this:

  1. Mouse hovers or the user uses the arrow keys to navigate the completion list
  2. activate-suggestion-by-index gets called
  3. It updates the model based on the outcome of event-updates-model?, which returns (not change-on-blur?), so the model does get updated
  4. component is redrawn, both external and internal models dont match - completion state is reset

A simple fix would be to change event-updates-model? to this:

(defn- event-updates-model?
  "Should `event` update the `typeahead` `model`?"
  [{:as state :keys [change-on-blur? rigid?]} event]
  (let [change-on-blur? (deref-or-value change-on-blur?)
        rigid? (deref-or-value rigid?)]
    (case event
      :input-text-blurred (and change-on-blur? (not rigid?))
      :suggestion-activated  false ;; (not change-on-blur?)
      :input-text-changed (not (or change-on-blur? rigid?)))))

And the final problem that I've hit is with the behavior of the ESC key, when :rigid? true - the input box gets cleared, but the model doesnt get reset. Relevant code:

(defn- reset-typeahead
  [state]
  (cond-> state
    :always clear-suggestions
    :always (assoc :waiting? false :input-text "" :displaying-suggestion? false)
    (event-updates-model? state :input-text-changed) (update-model nil)))

Problem is reusing the :input-text-changed, which in this particular case doesnt count as a state resetting worthy event. I would suggest introducing another event and extending event-updates-model? accordingly to always clear down.

orolle commented 6 years ago

168

I fixed some of the problems you mentioned above.

sveri commented 6 years ago

I have the same problem. To be honest I spent several hours trying to figure out what I did wrong as I think this is the most basic usecase regarding the editing of a field.

Gregg8 commented 5 years ago

PR #168 has been merged. Available in re-com 2.2.0.