danhunsaker / angular-dynamic-forms

Build Forms in AngularJS From Nothing But JSON (please see Alternatives in the README)
MIT License
379 stars 140 forks source link

Use . notation for ng-model to allow nested models #18

Closed kynan closed 10 years ago

kynan commented 10 years ago

The model for a form currently has to be completely flat i.e. every field must be an attribute of the form's ngModel. Forms cannot "reach into" subattributes because the lookup is via a single key. Fix this limitation by using a dot notation instead.

danhunsaker commented 10 years ago

I opted for the key notation for security purposes. You're right that it does flatten the model, though.

That said, this PR doesn't handle the assignment of values using val properly. Cannot consider for merge until it does.

kynan commented 10 years ago

I've added support for this now. Is this what you had in mind?

danhunsaker commented 10 years ago

That should work, yeah, though it would make a bit more sense, I think, to split the model name inside the function than out. Makes the function call a little easier to read.

I'll review the code a bit more thoroughly sometime tomorrow to make sure every use of field.model is being handled according to its designed purpose.

kynan commented 10 years ago

Indeed, splitting inside the function makes more sense. I've noticed I haven't changed the other places where field.model is used so I'll add that too.

danhunsaker commented 10 years ago

I just realized I misspoke earlier. The bracket notation was selected not for security, but because I can't guarantee every model property will be valid in dot notation. Reserved words, for example, cause issues when used as object properties in some JS engines (though I suppose this may have changed recently). So if we can continue using bracket notation, and still support nested models, that would be best.

I also screwed up on the implementations of the name attribute, I think. That should use bracket notation as well, so that regular browser form submits (as opposed to AngularJS submits) can be handled properly server-side.

kynan commented 10 years ago

If I understand you correctly, instead of

attrs.ngModel + "." + field.model

as I have done you'd prefer something along the lines of

attrs.ngModel + "['" + field.model.split('.').join("']['") + "']"
danhunsaker commented 10 years ago

Yeah, that looks about right. And something similar for the name attribute, though we won't want the first item to be bracketed, so that will be a bit more tricky.

kynan commented 10 years ago

I think I've now addressed all remaining issues:

I have also added examples of nested models to the demo and checked these are working.

danhunsaker commented 10 years ago

Awesome. I'll review your changes and probably merge them later today. Nested models will be really useful in the wild. :-)

danhunsaker commented 10 years ago

The rest looks good, though I was attempting to keep the demo templates consistent with each other aside from sorting. We'll have to update the README as well, but I don't mind handling that myself. Thanks!

kynan commented 10 years ago

Your two comments now addressed.

kynan commented 10 years ago

Is this now in good shape?

danhunsaker commented 10 years ago

Looks good! I need to fix a typo or two, cased by my own fat fingers, and update the demo/README files, but otherwise, thanks for the help, and sorry I took so long getting back to it.

kynan commented 10 years ago

Thanks!