reagent-project / reagent-forms

Bootstrap form components for Reagent
339 stars 78 forks source link

make reagent-forms re-frame compatible #127

Closed yogthos closed 6 years ago

yogthos commented 6 years ago

Allow the user to supply :doc, :get, :update!, and :save! keys when initializing. These could be hooked up to re-frame events by the user. In this mode, reagent-forms would not keep any internal state.

smogg commented 6 years ago

Is this still an open issue (I see those keys in the source code)? If yes, what exactly are we talking about here?

yogthos commented 6 years ago

Hi,

It's still an open issue. Currently, these keys get initialized by the bind-fields function here as:

{:doc doc
  :get #(deref (cursor-for-id doc %))
  :save! (mk-save-fn doc events)
  :update! (mk-update-fn doc events)}

The effort would be to allow passing this map that's initialized with custom functions for handling get/save!/update! functionality. These could be re-frame events or any externally managed source.

The tricky part here is the :doc key which currently provides access to the atom that contains the fields that the form is bound to. The :doc key is primarily used by the render-element macro to check whether an element is visible or not. I think that in the case where the the context map is supplied externally, the visible? function should be implemented in a way that doesn't require the state to be passed in.

smogg commented 6 years ago

Thanks for the clarification. That sounds non-trivial. I'm going to play with the source code and submit a (probably WIP) PR when I have something.

Here's how I imagine the form could look like when used with re-frame:

[forms/bind-fields
 [:input {:field :text
          :id :update-xyz}]
 {:save! (fn [id val] (re-frame/dispatch [id val]))
  :get #(deref (re-frame/subscribe [%]))}]

In which case on each field change, we would use its :id to run the user-provided save! function. In above example, the same :id would be used for re-frame/subscription so it would be nice to let the user specify that too (optionally):

[:input {:field :text
         :id :update-xyz
         :get-id :xyz}]

This will also require some additional work for handling namespaced keywords.

As for :visible?, we could make it behave similarly:

[:input {:field :text
         :id :update-xyz
         :visible? :xyz-visible?}]

Would run the previously specified :get fn with given subscription id:

(deref (re-frame/subscribe [:xyz-visible?]))

Does this make sense?

yogthos commented 6 years ago

Right, the get and save! functions would just map to subscriptions and dispatches in re-frame. I'm not sure I see the value in introducing a separate :get-id key. The :id is already used as a unique identifier for the field. The id->path function should handle namespaced keywords, as it splits on ., and calls keyword on the segments.

smogg commented 6 years ago

I think it might be common to have different naming conventions for dispatches and events, i.e. in re-frame's simple example we find :time-color-change for dispatch and just :time-color for a subscription. By letting the user specify only one keyword we are forcing them to use the same kw for both.

You're right about id->path, I misread the code.

yogthos commented 6 years ago

I think the idea would be to provide a single set of dispatch/subscribe events for the entire form though. So, even in this example the id would be passed in from the library:

{:save! (fn [id val] (re-frame/dispatch [id val]))
  :get #(deref (re-frame/subscribe [%]))}

Having a second optional id for getting would just add complexity without a clear benefit in my opinion. The users can always handle key name change outside the library if they wanted to as well.

smogg commented 6 years ago

Looks like we can close this.