HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Jquery sortable style function

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
stylejqueryfunctionsortable

Problem

This is a basic sorting function written in jQuery that moves items in the DOM around to create empty spaces for a droppable(). Since there is so much going on in a droppable over event, i'd love some feedback on how this could be done better.

This is all in a function called on the over event of the droppable()

I have an array of empty spots

var emptyHolders = [];

    $('.pipeline-holder').each(function(){
        if($(this).hasClass('holder-empty')){
            var eid = $(this).attr('id');
            emptyHolders.push(eid.substring(14));
        }
    });


cid is the droppable element that you're over top of, so I find the next open slot using

for (var i = 0; i  cid) {
            nextEmpty = currentEmpty;
            i = emptyHolders.length;
        } else {
            prevEmpty = parseInt(currentEmpty);
        }
    }


If there is a an empty slot further down the list, I use a for loop to loop through moving the items around the DOM to make the space needed to drop the element.

if (nextEmpty != null) {
        var moveMe = nextEmpty -1;

        for (var i = moveMe; i >= cid; i--) {

            var nextcount = i + 1;
            var me = $('#pipeline-rank-' + i);       
            var next = $('#pipeline-rank-' + i).parents('.pipeline-rank-row').next().find('.pipeline-holder');
            var pid = $('#pipeline-rank-' + i).find('.content-wrapper').attr('id');

            next.append($('#pipeline-rank-' + i).find('.content-wrapper'));
            next.removeClass('holder-empty');
            next.siblings('.remember-my-position-hover').html(pid);
        }
         $('#pipeline-rank-' + cid).addClass('holder-empty');
    }


If there is not one further down the list, I do the same thing in reverse to check if there is a spot above the droppable to push items up into.

It's usable here: http://jsfiddle.net/mstefanko/LEjf8/9/

Any thoughts are extremely appreciated!

Solution

Here you have some general thoughts:

-
First of all, cache jQuery elements (var $this = $(this); instead of multiple executions of $(this)). http://jsfiddle.net/tomalec/pXPdS/1/

-
You can also chain jQuery methods, it simplifies codes, and helps with above.

-
To reduce amount of code you can use toggleClass instead of addClass removeClass.http://jsfiddle.net/tomalec/pXPdS/2/

-
Instead of selecting $('#rankDrag') in start callback you can use reference given in arguments ui.helper
http://jsfiddle.net/tomalec/pXPdS/3/

-
To get id of empty/not empty holder ids, in my opinion, .map is more readable than .each. Moreover, you can move that function outside rearrange to avoid re-declaring it every time.

function getTruncatedId(index, element){
    return element.id.substring(14);
}

function rearrange(dropid) {
    //..
    emptyHolders = holders.filter('.holder-empty').map(getTruncatedId);
    isRanked = holders.filter(':not(.holder-empty)').map(getTruncatedId);
    //..
}


http://jsfiddle.net/tomalec/pXPdS/4/

-
Pass dropped element to rearrange, so you will not have to traverse through DOM again:

rearrange( $(this).attr('id') );//dropId = $(this).attr('id');
//..
isEmpty = $('#'+dropId).hasClass('holder-empty');


http://jsfiddle.net/tomalec/pXPdS/5/

Code Snippets

function getTruncatedId(index, element){
    return element.id.substring(14);
}

function rearrange(dropid) {
    //..
    emptyHolders = holders.filter('.holder-empty').map(getTruncatedId);
    isRanked = holders.filter(':not(.holder-empty)').map(getTruncatedId);
    //..
}
rearrange( $(this).attr('id') );//dropId = $(this).attr('id');
//..
isEmpty = $('#'+dropId).hasClass('holder-empty');

Context

StackExchange Code Review Q#25546, answer score: 3

Revisions (0)

No revisions yet.