Conversation
|
Still needs a test of a pair of connected sortables |
| if (!dragging || !_listsConnected(sortableElement, dragging.parentElement)) { | ||
| return | ||
| } | ||
| if (maxItems && _filter(_getChildren(sortableElement), _data(sortableElement, 'items')).length >= maxItems) { |
There was a problem hiding this comment.
I get your point: #257 (you talking about "class" confused me, I think it means something else outside of js). The clean solution would be to add a new function which filters children differently to only include items with a sortable-item-id.
Until we have this working, can you please add the -1 thingy and a comment //TODO: remove -1 once placeholder items are not included when getting children.
Otherwise people will have a bug.
There was a problem hiding this comment.
I think that would cause it's own bugs in other cases? If you have an element that's a child of your sortable but not supposed to be draggable. Maybe that's not a realistic use-case though.
There was a problem hiding this comment.
Hey I don't really understand how this would be a problem, can you explain it in more detail? I don't get what you mean. 😄
There was a problem hiding this comment.
We want to know how many items the sortable contains. If we just count the children of the sortable, we'll count the placeholder and anything else that's in there at the time—we don't know how many non-"items" are inside the sortable, so no constant offset will work in all cases.
If we filter the children by the items option we don't need an offset, but it only works if the user specified that option in a way that excludes the placeholder and anything else inside the sortable. This could be explicitly mentioned in the documentation.
I think your solution of filtering for sortable-item-id is the right thing to do, and lacking that change having no offset (and mentioning this in the doc) is second-best.
There was a problem hiding this comment.
Hey, I am fine with leaving it as long as we mention it in the docs. Could you add this to your PR? Thanks.
src/html.sortable.js
Outdated
| hoverClass: false, | ||
| debounce: 0 | ||
| debounce: 0, | ||
| maxItems: null |
There was a problem hiding this comment.
this should be 0 no null.
|
Hey @jamestwebber, looks great. Added some small comments. Are you still looking into writing tests or would you rather we create deal with them later? |
|
If you could write the tests that would be great, I don't think I can figure out the right structure while also removing jQuery usage. |
|
Hey @jamestwebber, sure, once we have the -1 or not part figured out I'll merge it without tests. One of us can write the tests later on. Maybe you find some more enhancements or bugs to fix were writing tests is easier (personally I find it really hard to write tests for UI elements where user interaction is required for the test). |
|
Thanks @jamestwebber 👍 |
3 participants
New PR to replace #257 because I borked the Git log