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

Objects Field (nested forms) #77

Merged
merged 4 commits into from
Aug 25, 2013
Merged

Objects Field (nested forms) #77

merged 4 commits into from
Aug 25, 2013

Conversation

lukeburns
Copy link
Contributor

Adds an objects field that allows you to nest forms within forms.

@ljharb
Copy link
Collaborator

ljharb commented Jul 25, 2013

This should ideally solve #11. However, I don't think creating a new form object is the ideal solution. I'll think about this one before reviewing for style. Also, please add many many tests :-)

@lukeburns
Copy link
Contributor Author

Above update fixes previously failing tests. Will add tests soon.

This solves #11 and allows the following behavior:

var reg_form = forms.create({
    username: fields.string({required: true}),
    password: fields.password({required: true}),
    confirm:  fields.password({
        required: true,
        validators: [validators.matchField('password')]
    }),
    personal: fields.object({
        name: fields.string({required: true, label: 'Name'}),
        email: fields.email({required: true, label: 'Email'}),
        address: fields.object({
            address1: fields.string({required: true, label: 'Address 1'}),
            address2: fields.string({label: 'Address 2'}),
            city: fields.string({required: true, label: 'City'}),
            state: fields.string({required: true, label: 'State'}),
            zip: fields.number({required: true, label: 'ZIP'})
        })
    })
});

What are your concerns?

@ljharb
Copy link
Collaborator

ljharb commented Jul 27, 2013

Primarily that "fields.object" doesn't feel intuitive to me. Why not just use a normal object literal?

@lukeburns
Copy link
Contributor Author

Just following the existing design pattern. Updated to take object literals. The following works:

personal: {
    name: fields.string({required: true, label: 'Name'}),
    email: fields.email({required: true, label: 'Email'}),
}

However, keeping nested fields as an object field might make things easier in the future. For example, say I'd like to wrap a group of nested fields with a fieldset tag. I would be able to pass in additional parameters and perhaps do something like this:

personal: fields.object({
    name: fields.string({required: true, label: 'Name'}),
    email: fields.email({required: true, label: 'Email'}),
}, { 
    widget: widgets.fieldset({label: 'Personal Information'}) 
})

I think it's worth keeping. What do you think?

@@ -167,3 +166,6 @@ exports.date = function (opt) {
return f;
};

exports.object = function (fields, opts) {
return forms.create(fields || {});
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please always add trailing newlines to files

@ghost ghost assigned ljharb Aug 1, 2013
@faceleg
Copy link

faceleg commented Aug 25, 2013

Has progress on this feature stalled?

@lukeburns
Copy link
Contributor Author

It's functional. Give it a shot.

@@ -23,13 +27,17 @@ exports.create = function (fields) {
b.toHTML = f.toHTML;
b.fields = {};
Object.keys(f.fields).forEach(function (k) {
b.fields[k] = f.fields[k].bind(data[k]);
b.fields[k] = data[k] ? f.fields[k].bind(data[k]) : f.fields[k].bind('');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain why this change is needed? (also it would seem simpler to do f.fields[k].bind(data[k] || ''))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't know. Reverting, because it works fine.

ljharb added a commit that referenced this pull request Aug 25, 2013
@ljharb ljharb merged commit 9aaf0b2 into caolan:master Aug 25, 2013
@ljharb
Copy link
Collaborator

ljharb commented Aug 25, 2013

I temporarily unmerged this. Posting a comment now.

});
b.data = Object.keys(b.fields).reduce(function (a, k) {
a[k] = b.fields[k].data;
return a;
}, {});
b.validate = function (callback) {
b.validate = function (obj, callback) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

obj isn't actually used anywhere in this function. i assume that's not intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fundamentally, the approach is emulate the field methods. When a nested form is validated, it is treated as a field and expects it to handle two arguments: form and callback. I'm not currently using the first argument, but it can be used in the future.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to do var form = obj || b so that we are using the passed form? Emulating the field methods isn't useful if it's not going to emulate the behavior as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is useful. If we remove the first argument, it will break. If we need the form object in the future, it's there. I'm open to alternative approaches, but I think there's a certain elegance to this solution.

@ljharb
Copy link
Collaborator

ljharb commented Aug 28, 2013

Remerged. I'm going to let this sit in master for a bit before bumping the version and publishing.

@smebberson
Copy link
Contributor

Can you use this functionality to add fieldset tags?

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

Successfully merging this pull request may close these issues.

4 participants