[Proposal] Ability to set max depth #40

Open
danielboendergaard opened this Issue Sep 12, 2013 · 16 comments

Comments

Projects
None yet
6 participants
@danielboendergaard

I think this could be a commonly used feature to set a maximum number of nested levels in the sortable list.

@johnny

This comment has been minimized.

Show comment
Hide comment
@johnny

johnny Sep 13, 2013

Owner

Thanks for pointing this out. Personally, I have not used it and the plugin is designed in such a way, that I do not think this is necessary:

  1. Sublists are not generated automatically
  2. There is the option isValidTarget where you can easily check, if the nesting is valid.

But I do think that this might be used often enough to warrant hacking up an example and adding it to the docs.

What do you think?

Owner

johnny commented Sep 13, 2013

Thanks for pointing this out. Personally, I have not used it and the plugin is designed in such a way, that I do not think this is necessary:

  1. Sublists are not generated automatically
  2. There is the option isValidTarget where you can easily check, if the nesting is valid.

But I do think that this might be used often enough to warrant hacking up an example and adding it to the docs.

What do you think?

@danielboendergaard

This comment has been minimized.

Show comment
Hide comment
@danielboendergaard

danielboendergaard Sep 13, 2013

I'm using IsValidTarget now and it works great, but I just thought that it might be something that could be simplified by adding max depth as an option.

In my application I have a max depth of 2. I have sublists on all my list items, because I want to be able to take a sublist item and put it in the root list and then put items into it, even if it had no subitems before.

There is the option to not include the sublist as you point out but I want the user to be able to put a list item in the sublist, just not a list item that has its own subitems.

Essentially if i specify a max depth of 2, I don't want the user to be able to do this:
example

While writing this i thought that it would make more sense to just rewrite my isValidTarget function to be generic, maybe someting like this can be added to the plugin or maybe just as an example.

isValidTarget: function ($item, container) {
    var depth = 1, // Start with a depth of one (the element itself)
        maxDepth = 2,
        children = $item.find('ol').first().find('li');

    // Add the amount of parents to the depth
    depth += container.el.parents('ol').length;

    // Increment the depth for each time a child
    while (children.length) {
        depth++;
        children = children.find('ol').first().find('li');
    }

    return depth <= maxDepth;
}

I'm using IsValidTarget now and it works great, but I just thought that it might be something that could be simplified by adding max depth as an option.

In my application I have a max depth of 2. I have sublists on all my list items, because I want to be able to take a sublist item and put it in the root list and then put items into it, even if it had no subitems before.

There is the option to not include the sublist as you point out but I want the user to be able to put a list item in the sublist, just not a list item that has its own subitems.

Essentially if i specify a max depth of 2, I don't want the user to be able to do this:
example

While writing this i thought that it would make more sense to just rewrite my isValidTarget function to be generic, maybe someting like this can be added to the plugin or maybe just as an example.

isValidTarget: function ($item, container) {
    var depth = 1, // Start with a depth of one (the element itself)
        maxDepth = 2,
        children = $item.find('ol').first().find('li');

    // Add the amount of parents to the depth
    depth += container.el.parents('ol').length;

    // Increment the depth for each time a child
    while (children.length) {
        depth++;
        children = children.find('ol').first().find('li');
    }

    return depth <= maxDepth;
}
@johnny

This comment has been minimized.

Show comment
Hide comment
@johnny

johnny Sep 18, 2013

Owner

Your example looks good. I will soon add a new example using this.

The reason for not adding this to the plugin itself is, that a fully general maxDepth function would need to load all subcontainers of the dragged item.

Thanks for your feedback.

Owner

johnny commented Sep 18, 2013

Your example looks good. I will soon add a new example using this.

The reason for not adding this to the plugin itself is, that a fully general maxDepth function would need to load all subcontainers of the dragged item.

Thanks for your feedback.

@danielboendergaard

This comment has been minimized.

Show comment
Hide comment

No problem :)

@johnny

This comment has been minimized.

Show comment
Hide comment
@johnny

johnny Sep 18, 2013

Owner

I will leave it open as a reminder.

Owner

johnny commented Sep 18, 2013

I will leave it open as a reminder.

@johnny johnny reopened this Sep 18, 2013

@johnny johnny referenced this issue Mar 5, 2014

Closed

maximum nesting #73

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Mar 6, 2014

We need this exact same feature (also with maxDepth = 2).
I coded it up, but this would be a nice feature to have.

The reason for not adding this to the plugin itself is, that a fully general maxDepth function would need to load all subcontainers of the dragged item.

Actually, I think this is a great reason why it should be included. This is not totally trivial to do. Moreover, the current API makes it not quite obvious to do this, as the item depth should be calculated once at onDragStart, stored and accessed for all later calls to isValidTarget, even though one could be tempted to calculate it in isValidTarget. Also, the lib doesn't currently store the container's depth, so we must loop, but this would probably be trivial to add at the lib level.

Note that the code @danielboendergaard gave is misleading as it won't quite work properly for maxDepth > 2.

tl;dnr: I'm 👍 to add this as a feature. I'd be glad to provide a PR if need be.

We need this exact same feature (also with maxDepth = 2).
I coded it up, but this would be a nice feature to have.

The reason for not adding this to the plugin itself is, that a fully general maxDepth function would need to load all subcontainers of the dragged item.

Actually, I think this is a great reason why it should be included. This is not totally trivial to do. Moreover, the current API makes it not quite obvious to do this, as the item depth should be calculated once at onDragStart, stored and accessed for all later calls to isValidTarget, even though one could be tempted to calculate it in isValidTarget. Also, the lib doesn't currently store the container's depth, so we must loop, but this would probably be trivial to add at the lib level.

Note that the code @danielboendergaard gave is misleading as it won't quite work properly for maxDepth > 2.

tl;dnr: I'm 👍 to add this as a feature. I'd be glad to provide a PR if need be.

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Mar 6, 2014

PS: This would also make the lib more robust. Currently, it's possible to drag a nested item from a group with nested: true to a group with nested: false. To me, nested: false implies maxDepth: 0 and the move should not be allowed.

PS: This would also make the lib more robust. Currently, it's possible to drag a nested item from a group with nested: true to a group with nested: false. To me, nested: false implies maxDepth: 0 and the move should not be allowed.

@Arthedian

This comment has been minimized.

Show comment
Hide comment
@Arthedian

Arthedian Mar 6, 2014

I have big problem with @danielboendergaard code, because its not working well. Problem is when I have

  <ol>
   <li>First
        <ol>
              <li>Second</li>
              <li>Third</li>
          </ol>
   </li>
  <li>Fourth</li>
 <ol>

and when i want to switch "Second" a "third" it doesnt do anything. Can i do anything with that?

I have big problem with @danielboendergaard code, because its not working well. Problem is when I have

  <ol>
   <li>First
        <ol>
              <li>Second</li>
              <li>Third</li>
          </ol>
   </li>
  <li>Fourth</li>
 <ol>

and when i want to switch "Second" a "third" it doesnt do anything. Can i do anything with that?

@danielboendergaard

This comment has been minimized.

Show comment
Hide comment
@danielboendergaard

danielboendergaard Mar 6, 2014

I fully support adding support for this in the core, which is why I made this post in the first place :)

@marcandre I'm curious why you say that my solution doesn't work for maxdepth > 2 since I'm using it in a project with maxdepth = 3 without issues.

However, I still don't think that my solution is optimal since it uses a lot of DOM traversing and I would rather see an implemetation in the core instead.

@Arthedian I don't really have a good answer for you since it is working for me, try to make a jsbin and share it here.

I fully support adding support for this in the core, which is why I made this post in the first place :)

@marcandre I'm curious why you say that my solution doesn't work for maxdepth > 2 since I'm using it in a project with maxdepth = 3 without issues.

However, I still don't think that my solution is optimal since it uses a lot of DOM traversing and I would rather see an implemetation in the core instead.

@Arthedian I don't really have a good answer for you since it is working for me, try to make a jsbin and share it here.

@Arthedian

This comment has been minimized.

Show comment
Hide comment
@Arthedian

Arthedian Mar 6, 2014

here is the link http://jsbin.com/ruvuqogo/1/edit i dont know why but its not working well on jsbin.

here is the link http://jsbin.com/ruvuqogo/1/edit i dont know why but its not working well on jsbin.

@danielboendergaard

This comment has been minimized.

Show comment
Hide comment
@danielboendergaard

danielboendergaard Mar 6, 2014

@Arthedian I just looked at your code and it works on jsbin if you correct the js errors it points out.

I think you should try to use the css from the docs and modify it to your needs, I find that the lists needs to have certain styling for the plugin to work properly.

@Arthedian I just looked at your code and it works on jsbin if you correct the js errors it points out.

I think you should try to use the css from the docs and modify it to your needs, I find that the lists needs to have certain styling for the plugin to work properly.

@Arthedian

This comment has been minimized.

Show comment
Hide comment
@Arthedian

Arthedian Mar 7, 2014

css from docs, and still not working: http://jsbin.com/ruvuqogo/6/edit

css from docs, and still not working: http://jsbin.com/ruvuqogo/6/edit

@marcandre

This comment has been minimized.

Show comment
Hide comment
@marcandre

marcandre Mar 8, 2014

@danielboendergaard the code doesn't always count the depth of $item correctly, in particular the following:

ol
  li ol
  li ol
    li

Here's the code to show that: http://jsfiddle.net/4ULSV/

@danielboendergaard the code doesn't always count the depth of $item correctly, in particular the following:

ol
  li ol
  li ol
    li

Here's the code to show that: http://jsfiddle.net/4ULSV/

@johnny

This comment has been minimized.

Show comment
Hide comment
@johnny

johnny Apr 19, 2014

Owner

@marcandre
If you could provide a PR for this feature, I will gladly merge it.

You are right, that this will make the lib more robust. You can replace the nested option with a maxDepth option.

Owner

johnny commented Apr 19, 2014

@marcandre
If you could provide a PR for this feature, I will gladly merge it.

You are right, that this will make the lib more robust. You can replace the nested option with a maxDepth option.

@johnny johnny added enhancement and removed docs labels Apr 19, 2014

@moschel26

This comment has been minimized.

Show comment
Hide comment
@moschel26

moschel26 Jan 18, 2016

@johnny
What you are making jquery sortable?
I need your help
http://stackoverflow.com/questions/34837423/failed-get-json-in-jquery-sortable
I've been trying and ask in the forums, but unresolved

Any suggestion to solve my problem?

Thank you

@johnny
What you are making jquery sortable?
I need your help
http://stackoverflow.com/questions/34837423/failed-get-json-in-jquery-sortable
I've been trying and ask in the forums, but unresolved

Any suggestion to solve my problem?

Thank you

@michaelsoriano

This comment has been minimized.

Show comment
Hide comment
@michaelsoriano

michaelsoriano Apr 14, 2016

+1 for maxdepth in nesting

+1 for maxdepth in nesting

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