Skip to content
This repository has been archived by the owner on Dec 12, 2021. It is now read-only.

Make the fields positionable, before, after, ... a given node (fix Issue #66) #70

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

elmatou
Copy link

@elmatou elmatou commented Aug 2, 2011

Hi (again) Ryan,

Here is my implementation of the positionable feature.
Make the fields positionable, before, after, at the top (prepend) or at the bottom (append) of a given node by adding attributes to the "add" link.

  f.link_to_add("Add", :tasks, :node => 'table', :position => :before)
# OR
  f.link_to_add("Add", :tasks, :node => '.tasks', :position => :top)

Fully tested with jQuery (and in use), not tested with Prototype.

EDIT : the choosen node should be unique (or you will get several new set of fields with same id's) and outside of any .fields set (or you won't be able to remove the added fieldset)

…at the bottom (append) of a given node. (Fully tested with jQuery, not tested with Prototype)
@p7r
Copy link

p7r commented Aug 11, 2011

This seems to be working great on adding a link, but the link_to_remove helper doesn't seem to behave nicely. If I add multiple nodes, removing any of them deletes all of below that node rather than just the node itself. This is because the adding seems to happen inside the fields div in a nested fashion, so rather than having:

<div class="fields">
  <div id="node_1">
     <!-- fields for nested form 1 -->
  </div>
</div>
<div class="fields">
  <div id="node_2">
    <!-- fields for nested form 2 -->
  </div>
</div>

We're getting this:

<div class="fields">
  <div id="node_1">
     <!-- fields for nested form 1 -->
  </div>
  <div class="fields">
    <div id="node_2">
      <!-- fields for nested form 2 -->
    </div>
  </div>
</div>

Looks like the insert needs to be after the nodes enclosing div with class="fields" node (i.e. it's parent node), rather than the node itself

@elmatou
Copy link
Author

elmatou commented Aug 11, 2011

Hi Paul,
I didn't change the link_to_remove implementation, and it "should" work the same way, by finding the closest '.fields' selector (the first parent tag of the link with the class 'fields') and removing it (more precisely hiding it, and change the _destroy field value.)

The point with my positionable feature is to choose carefully the insertion node (please provide your code if you want any help). '.fields' selector represent all fields for one nested item, by this, I mean that you should never insert new fields inside '.fields'.

These won't work (and will probably behave like you describe it) :

f.link_to_add("Add", :tasks, :node => '.fields', :position => :top)
f.link_to_add("Add", :tasks, :node => '.fields', :position => :bottom)

But these will work :

f.link_to_add("Add", :tasks, :node => '.fields', :position => :after)
f.link_to_add("Add", :tasks, :node => '.fields', :position => :before)

Anyway you shouldn't use '.fields' selector as it can missed if there isn't any child object in your rails resource. Rather than '.fields' you should use a container ('div.nested_items', ...).

Last but not list, are you using Jquery or Prototype ? I hadn't a chance to test it extensivly with Prototype.

@p7r
Copy link

p7r commented Aug 11, 2011

I think you misunderstand - I'm adding :after the #node_1, #node_2, etc.

I changed nested_form.js to the following, and it worked just fine:

    switch(insert_position){
    case 'before':
      var field = $(content).insertBefore(insert_node.parent());
      break;
    case 'after':
      var field = $(content).insertAfter(insert_node.parent());
        break;
    case 'top':
        var field = $(content).prependTo(insert_node.parent());
        break;
    case 'bottom':
        var field = $(content).appendTo(insert_node.parent());
        break;
    default:
        var field = $(content).insertBefore(this.parent());
    }

@elmatou
Copy link
Author

elmatou commented Aug 11, 2011

Hi,
I think, I understood your issue.
You shouldn't insert after, before, top or bottom of '#node1' or so, because these are inside your div with class 'fields'.
Be aware that 'fields' class, represent only 1 (one) item of your nested resource.

as I wrote : '.fields' selector represent all fields for one nested item, by this, I mean that you should never insert new fields inside '.fields'.
And that is precisly what you are doing by invoking :

f.link_to_add("Add", :tasks, :node => '#node1', :position => :after)

as '#node1' is the direct child of '.fields'

By changing the javascript you are only invoking :

f.link_to_add("Add", :tasks, :node => '.fields', :position => :after)

as '.fields' is the direct parent of '#node1'

BTW :

$(content).insertBefore(this.parent());

Won't work because this is not a jquery element.

I hope it is more clear.

@andrewajo
Copy link

Everything seems to be working fine for me until I put a fields_for inside of another fields_for. If I click link_to_add on the inner fields_for it adds it to all instances of the outer because the node name is the same...

@elmatou
Copy link
Author

elmatou commented Aug 26, 2011

I think I get your point.

Let's explain the way it insert fields :

In the master branch of nested_form, the fields position is related to the clicked link (in js this), which is a relative path with the link as root and before as position.

In my fork new fields position is directly related to :node => 'table' (or whatever your selector is), which should be an absolutepath (yes, like in your filesystem), because we want to be able to put the fields anywhere on the page.

You should choose a more presice and unique selector for the node (by adding an id or a class to the node), like :
:node => 'table.nested_set'.

If the selector is not unique (as it is in your case), Jquery&Prototype will add the html (our fields here) to each matching DOM elements, resulting in your symptoms...

I hope it will fix your issue.

@andrewajo
Copy link

Sorry I should have been more specific.

It is not an issue of having a unique node name - I have created a specific div with a unique class. Here is an example:

I am working on a database for a transportation company. So I have a nested_form for a Load. That load has many Pickup_Location, and each Pickup_Location has many Items.

When I add several Pickup_Locations it functions as it should.

The issue is that when I go to add Items to any one of these Pickup_Locations, because the Pickup_Locations that have been generated are identical, they all have the same node for Items.

This results in the new Item fields being added to each of the Pickups fields. Hopefully that is clearer?

The only thought I have had so far is to maybe identify which child it is in insert_node in the js file?

@elmatou
Copy link
Author

elmatou commented Aug 26, 2011

Ok, I get it.
I didn't use double nested fields, and I didn't treat this case...
Of course the position node for the second level will be created several times... not good.

The path should be related to the clicked link (as it is in the release), but in this case we loose the ability to attach the fields anywhere in the DOM, but only as a child, parent or siblings.

More accuratly, for now, the insertion node is related to the parent form of the link.

insert_node = $(this).closest("form").find(insert_node);

Can you gist your resulting HTML please, I don't have an handy app with double nested resources ?

@andrewajo
Copy link

Let me know if this is ok:

git clone git://gist.github.com/1173694.git gist-1173694

@andrewajo
Copy link

Think I found a fix:

insert_node = $(this).closest("form").find(insert_node);
child = $(this).closest(".fields").index();

// var field = $(content).insertBefore(this);
switch(insert_position){
case 'before':
var field = $(content).insertBefore(insert_node[child]);
break;
case 'after':
$(content).insertAfter(insert_node[child]);
break;
case 'top':
var field = $(content).prependTo(insert_node[child]);
break;
case 'bottom':
var field = $(content).appendTo(insert_node[child]);
break;
default:
var field = $(content).insertBefore(this);
}

haven't tested it yet...

@elmatou
Copy link
Author

elmatou commented Aug 26, 2011

I'm on something similar

if ($(this).closest(".fields").length) {    insert_node = $(this).closest(".fields").find(insert_node) }
else { insert_node = $(this).closest("form").find(insert_node) };

But it fails if you put the add link in a ".fields"

EDIT : Are you sure your solution work with simple nesting ? and without the node option ?

@andrewajo
Copy link

Will take a look at it and let you know - haven't had a chance to give it a thorough test...

@andrewajo
Copy link

I have switched to this and it works with simple nesting and without the node option:

if (insert_node != this) {
    insert_node = $(this).closest("form").find(insert_node);
    child = $(this).closest(".fields").index();
    insert_node = insert_node[child];
}

// var field = $(content).insertBefore(this);
switch(insert_position){
case 'before':
var field = $(content).insertBefore(insert_node);
break;
case 'after':
$(content).insertAfter(insert_node);
break;
case 'top':
var field = $(content).prependTo(insert_node);
break;
case 'bottom':
var field = $(content).appendTo(insert_node);
break;
default:
var field = $(content).insertBefore(this);
}

Still fails if the link is within a ".fields" though....

@samwgoldman
Copy link

I have solved this problem in a different way with my branch, but I think ryanb really need to merge fxposter's integration test branch before adding this kind of feature.

@elmatou
Copy link
Author

elmatou commented Aug 28, 2011

Hi Sam,
I agree with your point of view about testing.
Can you point your solution (a link to a commit or file) ?
I didn't found anything in your branch.

@samwgoldman
Copy link

Here's the comparison between our branches: master...samwgoldman:master

I merged in fxposter's branch, so the diff is a little messy. I think more useful is an example. I am using it here to create nested forms within tables[1]. Look at the 'fields_tag' and 'collection_tag' arguments.

  1. https://github.com/samwgoldman/gradebook/blob/master/app/views/evaluations/_form.html.haml

@elmatou
Copy link
Author

elmatou commented Aug 29, 2011

thx Sam,
It make sens, even if it is not "fully" postionnable, it is right that added fields will join a kond of"collection".

Anyhow, I will wait for an operational test suite before refactoring this part.

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

Successfully merging this pull request may close these issues.

None yet

4 participants