Sortable: Append a tr with td to the placeholder of tbody elements #1380
Conversation
When sorting tbody elements of a table the placeholder needs to have a tr with td elements to be visible. The appended elements are created in the same way as for the placeholder of a tr element; the first row of the sorted tbody is used for that.
|
Thanks for the patch. Please make sure to contribution guidelines which outline how to properly contribute a fix. You're missing the following:
I'm going to close this for now, but feel free to push updates and notify us and we'll reopen. If you have any questions, just ask here. Thanks. |
|
Hi, I updated my forked branch to match the style guide and added a test for tbody placeholders. I also signed the CLA and created a ticket: Please check if my PR is correct now and may be merged into master. |
|
This does seem to address the problem: http://jsfiddle.net/tj_vantoll/zL8uj2em/ Would it make sense to also include For this PR there are still some style violations, which you can find by running |
|
The HTML specifications do only allow one As for the style validations and the unit test: these were the 2 new commits to this PR. I think they were not appended while the ticket was closed. If I missed something else, please point it out :) |
|
@tjvantoll: may I ask for a short feedback? |
|
Sorry for the late response @oemmes. I have this on my list of things to do before the 1.12 release, which is still a little ways off. I don't see anything glaringly wrong, but I don't have time look at this in depth at the moment. |
|
@tjvantoll Were you still planning on reviewing this? |
| @@ -830,6 +831,16 @@ return $.widget("ui.sortable", $.ui.mouse, { | |||
|
|
|||
| }, | |||
|
|
|||
| _createTrPlaceholder: function(that, sourceTr, targetTr) { | |||
| that = that || this; | |||
There was a problem hiding this comment.
The first parameter is unnecessary. You can change this line to var that = this;.
|
@tjvantoll: Thanks for the feedback. A little explanation for the |
|
Ok that makes sense, and thanks for sticking with this. @scottgonzalez this looks good to me. |
| .appendTo( element ); | ||
| }); | ||
| if ( nodeName === "tbody" ) { | ||
| that._createTrPlaceholder( that.currentItem.find( "tr:first" ), $( "<tr></tr>", that.document[0] ).appendTo( element ) ); |
There was a problem hiding this comment.
- Avoid use of
:firstselector, use a filter method like.eq( 0 ). - Empty element construction uses just the opening name: $( "" )
- Missing spaces inside brackets.
- This line exceeds 100 characters.
|
@scottgonzalez I adjusted the code again. Hopefully it fits the guidelines now. |
|
Thanks, squashed and landed in master. |
When sorting tbody elements of a table the placeholder needs to have a tr with td elements to be visible. The appended elements are created in the same way as for the placeholder of a tr element; the first row of the sorted tbody is used for that. Fixes #10682 Closes gh-1380 (cherry picked from commit 962e05d)
When sorting tbody elements of a table the placeholder needs to have a tr with
td elements to be visible. The appended elements are created in the same way
as for the placeholder of a tr element; the first row of the sorted tbody is
used for that.