2
\$\begingroup\$

This is working code, but I want to simplify, correct, and improve the code.

window.addEvent = function(elem,type,callback) {
    var evt = function(e) {
        return callback.call(elem,e);
    };
    var cb = function(e) { return evt(e); };
    elem.addEventListener(type,cb,false);
    return elem;
};
window.findParent = function(child,filter,root) {
    do {
        if( filter(child)) return child;
        if( root && child == root) return false;
    } while(child = child.parentNode);
    return false;
};
function on(type, target, callback) {
  window.addEvent(document.body, type, function(e) {
    var s = window.findParent(e.srcElement || e.target, function(elm) {
        return elm.classList.contains(target);
    },this);

    if( s && callback ) {
        callback(e)
    }
  });
}
on("click", "page-link", function(e){
 console.log("hey its working");
});

In this example, I update the pagination with fetch, and I have to bind addEventlistener to every page link. I am thinking about, If its there a cleaner way, using the latest techniques? (with pure Javascript)

\$\endgroup\$
1
\$\begingroup\$

Response to statement

In this example, I update the pagination with fetch, and I have to bind addEventlistener to every page link.

Actually, in the example code it appears that elem.addEventListener() is called on document.body, not each link.

Variable Names

Some of the names are a bit misleading. For example target is used to check the class name of elements. Therefore a better name might be targetClass, target_class, etc. Similarly, s isn't very descriptive.

Feedback

Traversing DOM

Let's look at the findParent() function:

window.findParent = function(child,filter,root) {
    do {
         if( filter(child)) return child;
         if( root && child == root) return false;
     } while(child = child.parentNode);
     return false;
};

When would root ever evaluate to a false-y value? In the code above, document.body is passed, so that seems unlikely.

It might be simpler to use a while loop. Typically DOM traversing code I have seen would assign the child argument to a temporary variable (e.g. currentNode) and assign the parent node to that variable until the target node is found or the root is reached.

window.findParent = function(child,filter,root) {
    var currentNode = child;
    while(root != currentNode) {
        if (filter(currentNode )) {
            return currentNode ;
        }
        currentNode = currentNode.parentNode;
    }
    return false;
}

This could also be re-written as a for loop:

window.findParent = function(child,filter,root) {
    for(var currentNode = child; root != currentNode; currentNode = curentNode.parentNode) {
        if (filter(currentNode )) {
            return currentNode ;
        }
    }
return false;

Over-complicated addEvent

The addEvent function creates two functions (i.e. evt and cb) which basically just call callback passing the event object to the callback. These steps are superfluous. It could simply add the event listener, passing cb to the listener parameter:

window.addEvent = function(elem,type,callback) {
    elem.addEventListener(type,callback,false);
    return elem;
};

And does it need to return elem? That would support chaining but your example code doesn't assign that return value to anything or have a subsequent call...

Simplification

This looks like a great application for an event delegate, unless there is some requirement I am missing about splitting out the functionality into those separate functions? See the example below, which adds the event handler to the root and then only calls the callback if the target of the event passes the filter.

function on(eventType, targetClass, callback) {
  document.body.addEventListener(eventType, function (event) {
     if (event.target.classList.contains(targetClass)) {
       callback(event);
     }
  });
}

on("click", "page-link", function(e){
 console.log("hey its working", e.target.innerHTML);
});
<h1>Header Text</h1>
  <a class="page-link" href="javascript:void(0);">1</a>
  <a class="page-link" href="javascript:void(0);">2</a>
  <a class="page-link" href="javascript:void(0);">3</a>

\$\endgroup\$
  • \$\begingroup\$ this simplification made my day, there was no reason to split the code into multiple functions, I think it was easy to write. Thank you for the well explained answer, now I understand better. \$\endgroup\$ – almost okey Jan 30 '18 at 13:16
3
\$\begingroup\$

I think you can use querySelectorAll() - for example:

function on (type, target, callback) {
  document.querySelectorAll(target).forEach(function (elem) {
     elem.addEventListener(type, callback);
  })
}

Your function findParent seems similar to element.closest() . In case you need it to work with IE, you can use the code below. It works with a selector, not just a class name.

closest = (element, selector) => {
    while(element) {
        //For IE11
        if (!element.matches)
            element.matches = element.msMatchesSelector;

        if (element.matches(selector))
            return element;

        element = element.parentElement;
    }
}
\$\endgroup\$
  • \$\begingroup\$ with the sample usage of on(), document.querySelectorAll() would likely return no results when passed "page-link" unless there were elements with that tag name... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jan 30 '18 at 4:52
  • \$\begingroup\$ @SamOnela because in your findParent you find element using classList. You can change to ".page-link". Just my opinion, because pass and work with selector is better and flexible than just only 1 class. \$\endgroup\$ – DarknessZX Jan 30 '18 at 4:57
  • \$\begingroup\$ I am just saying that the OP would have to change the original sample code... or to use the code you suggested, would have to alter it to something like: document.querySelectorAll("." + target) \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Jan 30 '18 at 5:00

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.