Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Suggestions on link_to_add and blueprint functionality #216

Open
tiagoblackcode opened this Issue Nov 7, 2012 · 2 comments

Comments

Projects
None yet
3 participants

First of all, congratulations on such nice gem. I think this kind of feature should be deep inside the core of form builders as it is a very common problem when developing web applications.

I like this gem specially because it has a nice, clean approach. Although, there are a few suggestions I think should be considered on a next release (I also am able to help with that if it goes along).

First, I've noticed the new wrapper => false option that can be sent along the f.fields_for method call which is indeed very useful to provide custom containers to wrap the fields (specially when we want to wrap the fields in a table). Although it is very useful and clean, it gets a bit ugly when it comes to the javascript part.

Every time I want to have such custom wrapping options, I find myself overriding the window.nestedFormEvents.insertFields so it can find the proper container to add the fields into (taking the table example, the container would be something like table.purchase_products_fields tbody. Imagine if you have 3 or 4 nested models inside a form. You'll have to have some programming going on in the insertFields override until you get things right.

This leads my to the following suggestion: why not add the option to provide a container to the link_to_add function, which would keep the user-code simpler and less error-prone? This not only fits the above use case I mentioned, but also applies when the link_to_add is placed in something other than the nested fields container. Something like f.link_to_add "Add", :purchase_products, :container => "table.purchase_products_fields tbody" would be more than enough.

Secondly, and this is because I'm overwhelmingly meticulous is that the blueprint should be wrapped in something other than a div, because it is actually a template. My suggestion is to place it inside a <script type="text/template"></script> tag, which keeps the browsers from evaluating the DOM inside the script tag and keeps the template in a nice, clean container without any data-blueprint workaround going on.

Please note that this is are only suggestions and I am open to discuss this more specifically. I don't intend in any way to criticize the decisions taken which, as I said before, are part of a great job made by all the contributors. My perspective is to make things better and give you some feedback from someone that uses this gem quite a lot!

There are some assumptions that may be outdated as I haven't had the time to check the master branch, but I hope you get the point.

Best regards,
Tiago Melo

Collaborator

lest commented Nov 13, 2012

Thanks for suggestions. Here is my opinion about them:

  1. I'm open to adding new features to window.nestedFormEvents.insertFields but:
    • they should use data- attributes on a link, e.g.: f.link_to_add "Add", :purchase_products, :data => { :container => "table.purchase_products_fields tbody" } which helps us to avoid modifying link_to_add helper
    • they should be properly tested
  2. I'm not a fan of using <script type="text/template"></script> because people may unintentionally have script tags in their blueprints and it'll break html completely.

I gave the container functionality a shot in #278. Our use case was css conflicting when using the class "fields".

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