Conversation
…max_size parameter to automatically disable sortables at a certain length
extremely minor typo (lukasoppermann#251)
|
Hey @jamestwebber, thank you very much, I will have a look at it now. Are you saying you would need a connected test to know how to do it, or are you just not sure where to put it? If its the location, just create a new file: Also can you please to the following for me:
Thanks. 👍 |
| if (!dragging || !_listsConnected(sortableElement, dragging.parentElement) || _data(sortableElement, '_disabled') === 'true') { | ||
| return | ||
| } | ||
| if (maxItems && _filter(_getChildren(sortableElement), _data(sortableElement, 'items')).length >= maxItems) { |
There was a problem hiding this comment.
You need to do ....length - 1 otherwise if I set maxItems to 3, I can only add 2 items.
There was a problem hiding this comment.
This is because of the placeholder class getting added. If you pass an "items" option that doesn't match the placeholder, it works correctly this way. I think that's better than having it always off-by-one which seems confusing.
| } | ||
|
|
||
| // max items | ||
| if (options.maxItems && typeof options.maxItems === 'number') { |
There was a problem hiding this comment.
Couldn't you just remove this entire if block and use options.maxItemsin line 698? You could check for typeof number down there.
There was a problem hiding this comment.
Sure, I don't know if a type check is even necessary, just sort of emulating what I saw elsewhere. The type check probably isn't necessary at all, it will act weird if people give a weird parameter but that's on them.
|
Uh, that probably wasn't the right thing to do...I have a branch called maxitems with the correct history and changes but it's no longer attached to this PR. |
|
Hey @jamestwebber, no worries just create a new PR with the branch. (Or is there another issue I don't understand. ) |
3 participants
Here is the PR for maxItems (alone). I didn't write a test for this, I am not sure where the best place to put it would be. There are currently no explicit tests of connectWith that I could see, and that would be the starting point.