JavaScript Drag & Drop Sortable List

Question

I'm building a drag and drop sortable list in Javascript. This is what I have so far. Is this a good way to solve the problem? How can I improve my code?

<div id="list">
    <ul>
        <li data-value="Bananas" draggable="true">Bananas</li>
        <li data-value="Keys" draggable="true">Keys</li>
        <li data-value="Chocolate" draggable="true">Chocolate</li>
        <li data-value="Tomatoes" draggable="true">Tomatoes</li>
        <li data-value="Elephant" draggable="true">Elephant</li>
        <li data-value="Rugby" draggable="true">Rugby</li>
        <li data-value="Basketball" draggable="true">Basketball</li>
        <li data-value="Television" draggable="true">Television</li>
        <li data-value="Water" draggable="true">Water</li>
        <li data-value="Coffee" draggable="true">Coffee</li>
        <li data-value="Cake" draggable="true">Cake</li>
    </ul>
</div>

<script type="text/javascript">

    var array = [];
    var list = document.getElementById('list');

    for (i = 0; i < list.children[0].children.length; i++) {
        var getChildren = list.children[0].children[i].dataset.value;
        var getDataValues = getChildren;
        array.push(getDataValues);
    }

    document.addEventListener("dragstart", function(event) {
        event.dataTransfer.setData("dragPoint", event.target.attributes["data-value"].nodeValue);
        event.dataTransfer.setData("ID", event.target.id);
    });
    document.addEventListener("drag", function(event) {
        event.preventDefault();
        var targetElement = event.target || event.srcElement;
    });
    document.addEventListener("dragenter", function(event) {
        event.preventDefault();

        if ( event.target.tagName == "LI" ) {
          event.target.style.borderTop = "2px solid black";
        }
    });
    document.addEventListener("dragleave", function(event) {
        event.preventDefault();

        if ( event.target.tagName == "LI" ) {
          event.target.style.border = "";
        }
    });
    document.addEventListener("dragover", function(event) {
        event.preventDefault();
        event.dataTransfer.dropEffect = "move";

        var items_to_be_moved_on_drag = [];
        for (i = 0; i < list.children[0].children.length; i++) {
            var getChildren = list.children[0].children[i].dataset.value;
            var getDataValues = getChildren;

            if (event.target.attributes["data-value"].nodeValue != getDataValues) {
                items_to_be_moved_on_drag.push(getDataValues);
            }
        }
    });
    document.addEventListener("drop", function(event) {
        event.preventDefault();

        if ( event.target.tagName == "LI" ) {
            event.target.style.border = "";
        }

        var dragPoint = event.dataTransfer.getData("dragPoint");
        var dropPoint = event.target.attributes["data-value"].nodeValue;

        var arrayLength = array.length;
        var dragValue = array.indexOf(dragPoint);
        var dropValue = array.indexOf(dropPoint);
        var lastValue = array.indexOf(arrayLength);

        if (dragValue <= dropValue && dragValue !== dropValue) {

            var firstArray = array.slice(0, dragValue);
            var midArray = array.slice(dragValue, dropValue);
            var lastArray = array.slice(dropValue, arrayLength);

            midArray.shift();
            midArray.push(dragPoint);

            var newArray = firstArray.concat(midArray, lastArray);

            var getLI = list.getElementsByTagName('li');

            for (i = 0; i < getLI.length; i++) {
                getLI[i].attributes["data-value"].nodeValue = newArray[i];
                getLI[i].innerText = newArray[i];
            }

        } else if (dropValue <= dragValue && dragValue !== dropValue) {

            var firstArray = array.slice(0, dropValue);
            var midArray = array.slice(dropValue, dragValue);
            var lastArray = array.slice(dragValue, arrayLength);

            midArray.unshift(dragPoint);
            lastArray.shift();

            var newArray = firstArray.concat(midArray, lastArray);

            var getLI = list.getElementsByTagName('li');

            for (i = 0; i < getLI.length; i++) {
                getLI[i].attributes["data-value"].nodeValue = newArray[i];
                getLI[i].innerText = newArray[i];
            }

        }


    });

</script>

JSFiddle: https://jsfiddle.net/DumbMathBoy/6wvhpe74/

Answer 1

General coding points

• The type property in the script tag defaults to type="text/javascript" and is not required.
• Use a IIFE (Immediately Invoked Function Expression) to isolate your code from the global scope.
• Use === strict equality rather than == Same for !== rather than !=
• Use functions to remove repeated and verbose API calls. See rewrite.
• Not sure why you wrapped the list in an unnecessary div.
• Inline functions such as event listeners may be better defined as arrow functions (less code noise)
• Use const for variables that do not change.
• Avoid setting element styles directly. Define CSS rules to hold the style variants by class names and modify element class names to change the style. (see rewrite)

Why?

You have

>        var getChildren = list.children[0].children[i].dataset.value;
>        var getDataValues = getChildren;
>        array.push(getDataValues);

I do not understand why you move the same value from getChildren to getDataValues. You don't need the intermediate variables. You do the same in drag over. You can just use.

        array.push(list.children[0].children[i].dataset.value);

Bug

You have not checked that drag event may be outside the list, resulting in an error thrown. Always ensure you test code before release.

Design

Though on the most part your code works it lacks some user feedback that makes it difficult to use.

• There is zero indication that items are draggable. Change the cursor, use hover effect to show what mouse is over.
• Use drag drop API to show where and where not you can drag an item. If you don't preventDefault in the dragover event the cursor will show no-drop
• While dragging there is no way to know what item is being dragged. You can use an image, or change the dragged item style to give the needed feedback.

The code is way to complex. Especially the drop code.

Because you are only drag between positions in the list you don't need to use the dataTransfer to track the data you are dragging. A simple JS variable can hold the dragged element use dataTransfer to control user feedback.

You don't need to locate the insert pos of the dragged name. The array holding names can be built by iterating the list items of UL. A one liner rather than all that drop code.

You don't need to use dataset from within JS code. You can define properties for DOC elements just like any JS Object. (see example)

Rewrite

• The rewrite creates the list when needed from the data stored in the unsorted list item.
• There is a set of simple helper functions to remove the verbosity and longwindedness of common DOM tasks

;(()=>{
    /* Helper functions to reduce verbosity of DOM API */
    const isArr    = Array.isArray;
    const clsActs  = Object.freeze({"+": "add", "-": "remove", "!": "toggle"});
    const tag      = (tag, props = {}, sProps = {}) => style(Object.assign(document.createElement(tag), props), sProps);
    const style    = (el, style) => (Object.assign(el.style, style), el);
    const append   = (el, ...sibs) => sibs.reduce((p, sib) => ((isArr(sib) ? Object.append(p, ...sib) : p.appendChild(sib)), p), el);
    const addAbove = (el, ref, ...sibs) => sibs.reduce((p, sib) => ((isArr(sib) ? Object.append(p, ref, ...sib) : p.insertBefore(sib, ref)), p), el);
    const remove   = (el, ...sibs) => sibs.map(sib => el.removeChild(sib));
    const qry      = (qStr, el = document) => el.querySelector(qStr);
    const qryAll   = (qStr, el = document) => [...el.querySelectorAll(qStr)];
    const listen   = (el, names, call, opt = {}) => ((isArr(names) ? names : [names]).forEach(name => el.addEventListener(name, call, opt)), el);
    const clsAct   = (el, act, name) => (clsActs[act] ? (el.classList[clsActs[act]](name), el) : el);
       
    var dragItem;
    const listEl = qry("#list");
    const names = listEl.dataset.itemNames.split(",");
    const updateNames = () => {
        names.length = 0;
        names.push(...qryAll("#list > li").map(li => li._name));
    };
    append(listEl, ...names.map(name => tag("li", {
            textContent: name, draggable: true, className: "dragList", 
            _name: name, _dropable: true  /* Note _ before property names to protect future compatibility (NO NAME CLASHES) */
        })
    ));
    const endDrag  = e => { clsAct(dragItem, "-", "dragging") }
    const dropItem = e => {
        if (e.target._dropable ) {
            clsAct(e.target, "-", "over");            
            if (e.target !== dragItem) {            
                addAbove(listEl, e.target, ...remove(listEl, dragItem));
                updateNames();
            }
        }
    }
    const enterLeave = e => {
        if (e.target._dropable) {  
            e.dataTransfer.dropEffect = "move";
            clsAct(e.target, e.type === "dragenter" ? "+" : "-", "over");
            e.preventDefault();
        }
    }
    listen(document, "dragstart", e => {
        dragItem = e.target;
        clsAct(e.target, "+", "dragging");
        e.dataTransfer.effectAllowed = "move";
    });    
    listen(document, ["dragenter", "dragleave"], enterLeave);
    listen(document, "dragover",  e => { e.target._dropable && e.preventDefault(); });
    listen(document, "dragend",  endDrag);
    listen(document, "drop",  dropItem);
})();

.dragList {
  cursor: grab;
}
.dragList:hover {
  background: #DEF;
}
.dragList.over {
  border-top: 2px solid black;
}
.dragList.dragging {  
  color: #DDD;
}

<ul  id="list" data-item-names="9 Bananas,8 Chocolate,7 Tomatoes,5 Rugby,6 Basketball,4 Television,3 Water,2 Coffee,1 Cake">
</ul>