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

Add Form.NestForm template to allow different theming for nested forms used in Object editor #392

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rjmackay
Copy link
Contributor

Adds Form.NestedForm and uses it in editors/Object. NestedForm extends Form but uses a different template which doesn't have a form tag. This generates valid HTML, but leaves the flexibility to tweak the nested form template if needed.

Adds Form.NestedForm and uses it in editors/Object.
NestedForm extends Form but uses a different template which doesn't
have a form tag. This means we're now generating valid HTML.
…present

Otherwise we get errors when validating fields that weren't rendered
@philfreo
Copy link
Collaborator

+1 this is a great way to do it - I've done similar things in 2 different projects

However you need to make the same logic in src/editors/nestedmodel.js

Doing this also makes it super easy to have a nice InlineNestedModel List Item editor -- rather than having it use a Modal.

@fonji
Copy link
Contributor

fonji commented Jun 26, 2014

+1 I need that!

@powmedia
Copy link
Owner

Looks like a much needed improvement however I've got several tests failing with these changes; is are you able to update these?

After the merge would be great to simplify the list editor so modals aren't required as @philfreo says

@fonji
Copy link
Contributor

fonji commented Jun 26, 2014

I believe this implies a small modification of the bootstrap3 templates.
For example,

Form.editors.Base.prototype.className = 'form-control';

should be replaced by something similar to this:

Form.editors.Base.prototype.className = function() {
  if (this.hasNestedForm)
    return '';
  return 'form-control';
};

@rjmackay
Copy link
Contributor Author

@fonji except NestedModel doesn't set hasNestedForm so I'm not sure that works? Maybe I should just unset className for specific editor types?

@@ -44,6 +44,9 @@
');

Form.editors.Base.prototype.className = 'form-control';
Form.editors.Object.prototype.className = '';
Form.editors.NestedModel.prototype.className = '';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure this is correct. I don't know enough about bootstrap, or have anywhere setup to test. @fonji maybe you could confirm?

Copy link
Contributor

Choose a reason for hiding this comment

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

I confirm it works and is nicer than my solution.
Please ask if you want details about those lines.

I still think there's some work to do on those templates, especially for the object type, as the fields titles are hidden.
Here's a fiddle.

@rjmackay
Copy link
Contributor Author

Updated to fix unit tests, and simplified NestedModel while at it.
I did run into some issues running tests in Firefox, so maybe someone else could double check that?

@fonji
Copy link
Contributor

fonji commented Jun 27, 2014

@rjmackay Will do.

@fonji
Copy link
Contributor

fonji commented Jun 27, 2014

I had errors with every browser I used for the tests (firefox on linux and windows, chromium on linux, safari on windows and IE).
I don't have the time to debug now, sorry.

@philfreo
Copy link
Collaborator

Here's an example of using this with a List editor now

http://jsfiddle.net/dW2Qu/484/

Either we should get rid of the Modal adapters or we should add an InlineNestedModel like in the jsfiddle above

@fonji
Copy link
Contributor

fonji commented Jul 7, 2014

I tried testing once more and have good news: tests actually works on most tested browsers.
I still have trouble with... IE, for a change. I'll keep you guys up to date when I found what's bothering it.
Edit: IE tests failing aren't related to this merge. See my next comment.

Bad news is: I'm an idiot. I git-cloned an old version of @rjmackay's repository and that's why it didn't work. I guess I forgot how to git remote. Shame on me.
Edit: I know why I am an idiot. I cloned the master branch. Very usefull...

@fonji
Copy link
Contributor

fonji commented Jul 7, 2014

The problems with IE are focus and blur-related and have nothing to do with these changes.
They also happen with powmedia/backbone-forms/master.

@fonji
Copy link
Contributor

fonji commented Jul 9, 2014

I made a small change to allow each instance and subclass to set their own NestedForm and NestedField class.
I'm not sure it's usefull so I didn't write any tests yet. Please let me know if you're interested, I'll write some and create a pull request to @rjmackay's version.

Edit: usage example:

Backbone.Form.editors.List.InlineObject = Backbone.Form.editors.Object.extend({});
Backbone.Form.editors.List.InlineObject.NestedField = Backbone.Form.Field;

So the applied template is the same.

@slackjaw
Copy link

slackjaw commented Aug 1, 2014

+1. I could put this utility to use right away - but how specify the template for the nestedmodel? I need an inline list of objects, like in @philfreo's fiddle, but I need to style it with a template.

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

Successfully merging this pull request may close these issues.

5 participants