sir-dunxalot / ember-easy-form-extensions

Manages form submission in the controller/component and route layers of Ember apps
MIT License
28 stars 14 forks source link

take form className from configuration #13

Closed Iftahh closed 9 years ago

Iftahh commented 9 years ago

I want the default form to be with a class "form-horizontal", but the form-wrapper ignores the formClass set in the configuration

in app.js:

Ember.EasyForm.Config.registerWrapper('horiz', {
      formClass: 'form-horizontal',   
      // other classes

in my template:

{{#form-wrapper wrapper="horiz" }}     <!--   rendered form has the wrong default "form" class -->
     {{input firstName}}
     {{input lastName}}
{{/form-wrapper}}

since the default is ignored as a workaround I have to set the class manually:

{{#form-wrapper wrapper="horiz" className="form-horizontal" }}
    ...     
sir-dunxalot commented 9 years ago

Thanks for opening the issue. form-wrapper is just a component - it's not the same as easy form's {{form-for}} helper and doesn't support the same arguments being passed. As you discovered, wrapper isn't passed to it. The content of a block component is executed in the scope of the surrounding controller, not the component, so there wouldn't be a public way to access the wrapper in other views like {{input}} if wrapper was passed to the component - that's why it was left out of this addon.

You can set an app-wide default class name like so:

// app/components/form-wrapper.js

import FormWrapperComponent from 'ember-easy-form-extensions/components/form-wrapper';

export default FormWrapperComponent.extend({
  classNames: 'form-horizontal' // Overrides `form`
});

Individual className defaults are overridden as you did in your example above:

{{!-- app/templates/some-route.hbs --}}

{{#form-wrapper className='form-horizontal'}}
  {{!-- Content here wouldn't have public access to wrapper--}}
{{/form}}

Possible solutions here are:

Iftahh commented 9 years ago

Thank you for the quick and well written response !

On Sun, Mar 29, 2015 at 6:45 PM Duncan Walker notifications@github.com wrote:

Thanks for opening the issue. form-wrapper is just a component - it's not the same as easy form's {{form-for}} helper and doesn't support the same arguments being passed. As you discovered, wrapper isn't passed to it. The content of a block component is executed in the scope of the surrounding controller, not the component, so there wouldn't be a public way to access the wrapper in other views like {{input}} if wrapper was passed to the component - that's why it was left out of this addon.

You can set an app-wide default class name like so:

// app/components/form-wrapper.js import FormWrapperComponent from 'ember-easy-form-extensions/components/form-wrapper'; export default FormWrapperComponent.extend({ classNames: 'form-horizontal' // Overrides form });

Individual className defaults are overridden as you did in your example above:

{{!-- app/templates/some-route.hbs --}} {{#form-wrapper className='form-horizontal'}} {{!-- Content here wouldn't have public access to wrapper--}}{{/form}}

Possible solutions here are:

  • Create an instance of {{form-for}} that works with HTMLBars (I'm probably against this)
  • Add clearer documentation on how to set default classNames (as above)
  • Deprecate the formClass option in easy form wrappers (and display a warning if it's used)
  • Create a new way of setting classNames (e.g. a hash in the config/environment.js file
  • Use private properties to enable the wrapper option to be passed to {{#form-wrapper}}

— Reply to this email directly or view it on GitHub https://github.com/sir-dunxalot/ember-easy-form-extensions/issues/13#issuecomment-87430464 .

sir-dunxalot commented 9 years ago

My pleasure. Which of the possible solution do you think would best fit with your use cases? Personally, I'd prefer a mix of 2, 3, and 4.

Iftahh commented 9 years ago

Sorry, I'm just beginning learning Ember so I can't comment on how to implement a solution. But however you choose to solve it - setting default classes is definitely important so a clear documentation for it is a must, and deprecation warning for invalid config is useful as well.

sir-dunxalot commented 9 years ago

No problem. I'll go ahead with the two things you suggested and I'll look for other opportunities to tidy up the use of wrappers with this addon.

mistahenry commented 9 years ago

why are you against Create an instance of {{form-for}} that works with HTMLBars (I'm probably against this)

sir-dunxalot commented 9 years ago

I think there are more elegant ways of executing the form element. If you're using {{form-for model}} then there's not really a need to pass in the model at all. If you're using {{form-for someOtherModel}} then I'd question the restfulness of the route. If you're using both of the above models on a single form, which is definitely an applicable use case, each set of inputs should be surrounded by a <fieldset>, not <form> so {{form-for}} falls short here.

I definitely support some kind of form wrapping element but have never used {{form-for}} in 18 months of Ember. I'm definitely open to hearing the other side of the discussion, of course.

mistahenry commented 9 years ago

First, I'd like to thank you for carrying this library along and turning it into an addon useable with HTMLBars and 1.10. I totally agree with what you just said about form-for as long as ember-validations quasi supports validation on nested objects since model's properties are now explicitly prefaced by model. due to the deprecation of ObjectController.

Let me just throw a few ideas at you that I implemented and you can let me know if you'd want me to fully genericize them in a pull request.

1). Forcing me to use a legend in form-controls component has lead me to create my own form-content component that wraps all my inputs with the right classes. Requiring a legend is a personal preference.

2). My forms often take the process of a create, review, complete process. I represent each of these steps with a different subroute. The object being validated is the parent route's model, and each child route gets the model via this.modelFor('parent'). Thankfully I am able to use nested properties on the model for validations:

validations: {
        'model.amount': {
            presence: true
        }
    }

Now in the review process, I realized that a readonly version of the form would be super beneficial since I wanted to keep the label formatting the same and just show the value's of the inputs on the page before. I could just put my form body ie the fields into a template and render it as a partial into each of the steps, with someway to denote the form is readonly.

I achieved this by adding a readonly property to my form-content component that defaults to false when not present. I override the form-content component's yield function:

_yield: function(context, options, morph, blockArguments) {
        var view = options.data.view;
        var parentView = this._parentView;
        var template = get(this, 'template');

        if (template) {
            Ember.assert("A Component must have a parent view in order to yield.", parentView);

            view.appendChild(Ember.View, {
                isVirtual: true,
                tagName: '',
                template: template,
                _blockArguments: blockArguments,
                _contextView: parentView,
                _morph: morph,
                context: get(parentView, 'context'),
                controller: get(parentView, 'controller'),
                formContent: view
            });
        }
    }

to provide the children ie the {{input}} with a reference to the formContent component without manually passing it to each child. This yield is very specific to the Ember version.

In my customized input template, which I point to from the wrapperConfig hash, I can check:

{{#if _view._parentView.formContent.readonly}}
        {{read-only-input propertyBinding='view.property'}}
    {{else}}
...regular workflow with hints and errors and input-field
{{/if}}

And then the read-only-input helper defined in my initializer:

Ember.EasyForm.ReadOnlyInput = Ember.EasyForm.BaseView.extend({
            tagName: 'div',
            classNames: ['readonly-form-value'],
            templateName: Ember.computed.oneWay('wrapperConfig.readOnlyTemplate'),
            displayValue: function(){
                return this.get('formForModel').get(this.get('property'));
            }.property('property')
        });

        Ember.Handlebars.registerHelper('read-only-input', function(property, options) {
            options = Ember.EasyForm.processOptions(property, options);
            if (options.hash.propertyBinding) {
                options.hash.property = EasyFormShims.getBinding(options, 'property');
            }
            if (options.hash.inputOptionsBinding) {
                options.hash.inputOptions = EasyFormShims.getBinding(options, 'inputOptions');
            }
            var context = this;
            return EasyFormShims.viewHelper(context, Ember.EasyForm.ReadOnlyInput, options);
        });

Seems to be working so far, but I just wrote this morning so I haven't really tested it thoroughly. But now I can my create template:

{{#form-content}}
  {{partial 'form-fields'}}
{{/form-content}}

and my review and complete:

{{#form-content readonly=true}}
  {{partial 'form-fields'}}
{{/form-content}}

Still needs to support selects and checkboxes....gonna figure that out later.

3). I wanted to display an asterisk and apply a particular style to labels of required fields. This did the trick:

Ember.EasyForm.Label.reopen({
            //Heavily dependent on Ember Validation Internals
            showRequired: function(){
                var self = this;
                var validations = this.get('controller').get('validators');
                var required = false;
                if(validations){
                    validations.forEach(function(val){
                        if(val.property === self.property && val.options.message === Messages.defaults['blank']){
                            required = true;
                        }
                    });
                }
                return required;
            }.property()
        });

I wasn't sure how to get that the property was being validated for presence other than by the message. Getting the constructor didn't really give me a string I liked working with. Probably could be better but I didn't want to touch validation internals.

sir-dunxalot commented 9 years ago

All of the issues identified by @henryw14 and @Iftahh are tackled as part of the major upgrade for Ember 1.11 and above. You can test the prerelease here. Please note the API for inputs has changed slightly from the original Easy Form syntax.

sir-dunxalot commented 9 years ago

Feedback on the new setup would be very much appreciated.

sir-dunxalot commented 9 years ago

Fixed by #37