jonjamz / blaze-forms

Dead easy reactive forms with validation (Meteor).
https://atmospherejs.com/templates/forms
MIT License
113 stars 11 forks source link

Support instantiating a form block with a single config object #24

Closed jonjamz closed 9 years ago

jonjamz commented 9 years ago

This might be a little easier to pick up initially--it's more semantic, and the action doesn't need to come from a helper that returns a function, which can be a source of confusion during initial use.

Template['testForm'].helpers({
  formConfig: function () {

    var schema = new SimpleSchema({
      testField: {
        type: String,
        max: 3,
        instructions: "Enter a value!"
      }
    });

    var action = function (els, callbacks, changed) {
      console.log("[forms] Action running!");
      console.log("[forms] Form data!", this);
      console.log("[forms] HTML elements with `.reactive-element` class!", els);
      console.log("[forms] Callbacks!", callbacks);
      console.log("[forms] Changed fields!", changed);
      callbacks.success();
    };

    return {
      schema: schema,
      action: action
    }

  } // EO formConfig
});
steph643 commented 9 years ago

Well, I am not sure. Here is my experience:

At first, when using your package, I was annoyed by the fact that there were 3 arguments (data, action, schema) instead of 1. This was clearly different from what I expected and looked overcomplicated.

Also, later on, I had to add custom arguments to forms, hence there were a mix between my own arguments and your 3 arguments. This didn't look so good, especially because your argument names are very generic (I though formData, formAction and formSchema would have been better).

I was also disturbed by the action helper returning a function: this was the first time I had to write a helper directly returning a function and it looked weird on the screen.

So, at that time, I wondered why you didn't put all arguments in a single object.

But then I thought that clearly you can't pass data in an object, as data is usually taken from the template context. It would be awkward to force the user to go through a helper. So anyway you need 2 arguments, why not 3?

Another point: with the current API, you can use the same code to provide the schema to several forms, while in your proposal this is not possible anymore in a simple manner. For example, if you have several forms for the same collection2, you can use this global helper to provide schema:

Template.registerHelper('gh_postSchema', function() {
    return Posts.simpleSchema();
    });

Here is what the change would look like in my actual code:

Template.projectSettings.helpers(
    {
    th_projectSchema: function()
        {
        return Projects.simpleSchema();
        },

    th_submitFunction: function()
        {
        var id = this.project._id;
        var instance = Template.instance();
        return function(els, callbacks, changed)
            {
            // Update the project
            if (!_.isEmpty(changed))
                Meteor.call('Projects.mutate.update', id, changed);

            // form API
            callbacks.success();

            // Close the modal
            // Cannot use data-dismiss on a submit button. See http://stackoverflow.com/questions/22679867/dismiss-and-submit-form-bootstrap-3
            instance.$('.modal').modal('hide');
            };
        }
    });
Template.projectSettings.helpers(
    {
    formConfig: function()
        {
        var id = this.project._id;
        var instance = Template.instance();
        return {
            schema: Projects.simpleSchema(),
            action: function(els, callbacks, changed)
                {
                // Update the project
                if (!_.isEmpty(changed))
                    Meteor.call('Projects.mutate.update', id, changed);

                // form API
                callbacks.success();

                // Close the modal
                // Cannot use data-dismiss on a submit button. See http://stackoverflow.com/questions/22679867/dismiss-and-submit-form-bootstrap-3
                instance.$('.modal').modal('hide');
                }
            }
        }
    });

The first version looks better to me. For example, in the new version, see how var id and var instance are forced to be far away from the action function itself...

So I agree your initial API looks a bit unconventional, but in the end I think you were right.

jonjamz commented 9 years ago

You make a very good case against this change.

jonjamz commented 9 years ago

Decided this is unnecessary. Will keep the arguments the same.