Removal of jQuery from script to auto-apply coupons from a link

Question

We are using a small extension that auto-applies coupons from a link.

The extension had pop-up JS based on jQuery:

<script>
    jQuery.noConflict();
    jQuery(function() {

        var appendthis = ("<div class='modal-overlay js-modal-close'></div>");

        jQuery(document).ready(function(e) {
            //e.preventDefault();
            jQuery("body").append(appendthis);
            jQuery(".modal-overlay").fadeTo(500, 0.7);
            //$(".js-modalbox").fadeIn(500);
            var modalBox = 'popup1';
            jQuery('#' + modalBox).fadeIn(jQuery(this).data());
        });


        jQuery(".js-modal-close, .modal-overlay").click(function() {
            jQuery(".modal-box, .modal-overlay").fadeOut(500, function() {
                jQuery(".modal-overlay").remove();
            });

        });

        jQuery(window).resize(function() {
            jQuery(".modal-box").css({
                top: (jQuery(window).height() - jQuery(".modal-box").outerHeight()) / 2,
                left: (jQuery(window).width() - jQuery(".modal-box").outerWidth()) / 2
            });
        });

        jQuery(window).resize();

    });
</script>

It was our highest traffic page, in order to increase page speed we were removing unnecessary libraries and jQuery was one of them. So we needed to turn this code into pure JS.

The dev commented:

As far as I can see, this script performs a very simple task in a very complicated way.

First: there is no need to use fadeIn\fadeOut methods in this case. These methods freeze browser in loading moment. We need to use CSS transitions and just add opacity.

Second: there is no need to change top and left params during resize. We can use CSS and make the browser do it for us.

He did small changes to CSS of the extension, and final pure JS equivalent is:

<script>
    window.onload = function() {
        document.getElementById("popup-wrapper").className += " modal-overlay_visible";
    }

    function closeMethod() {
        document.getElementById("popup-wrapper").className += " modal-overlay_hidden";
    }

    function DOMready() {
        var closeElements = document.getElementsByClassName("js-modal-close");
        Array.from(closeElements).forEach(function(element) {
            element.addEventListener('click', closeMethod);
        });
    }
    document.addEventListener("DOMContentLoaded", DOMready);
</script>

My response to the comments asking why both load and DOMContentLoaded are used:

Yes, there is a reason to use DOMContentLoaded.

Since until the DOM is built, we can not add events to the elements, they simply do not exist.

For a sample, if I tried to get in global scope document.getElementsByClassName("modal-overlay__close"); this instruction would return []

Also, we can not show the popup until all popup styles and popup picture are loaded.

That's why, I use window load event.

Answer 1

As it stands, in most ways the code is now much better than the original. Your senior dev is absolutely correct in wanting to use CSS for the modal size. Despite this, there are a few suggestions I would make.

Instead of using window.onload use window.addEventListener("load", function(){});. If window.onload is used, then the function activating the modal could be overwritten by accident in the future.

There is no point in including Method in closeMethod. I'm sure that Method was added when the dev realized that just naming the function close hid the window.close function. closeModal would be a more descriptive name that also avoids this problem.

Is there any reason that both load and DOMContentLoaded events are listened for? These events should fire incredibly close to each other, and the code can likely be combined into a single initialization method to reduce complexity.

The ID of the popup-wrapper is overdefined. More than one method has it hardcoded in. If it were to be changed, it would be fairly easy to forget to change one or more occurrences. To fix this, it might be best to wrap everything in an immediately invoked function expression (IIFE) and pass the ID in. Using an IIFE also makes it possible to trivially avoid any troubles with global scope.

Instead of appending to className, use the classList API as this also makes removing the modal-overlay_visible class trivial. It also might make it possible to remove the modal-overlay_hidden class.

The DOMready function is exposed to the global scope. This isn't a huge issue on small pages, but should still be avoided to increase maintainability.

Lastly, consistency is a good thing. A style guide can make it easier to read code by ensuring everyone sticks to using double quotes (or single quotes) and uses the same CSS class patterns (I expected js-modal-close to be modal-overlay_close)

I could provide several different ways of writing this module. Which way is best heavily depends on your situation and is a judgement I can't make.

Answer 2

You don't need the element to exist before applying event handlers. For example, in jQuery, rather than writing your handlers in a $(function(){...}) you can attach the handlers to the document and wait for them to bubble instead, like so..

So, in jquery,

$(function(){ $("something").click(function(){...}); })

becomes...

$(document).on("click", "something", function(){...})

The benefit is that you don't have to wait for the DOM and you don;t have to loop each element and assign a whole bunch of event handlers..

Your current updated vanilla JS code is duplicating that event handler function in memory for each and every element you apply it to. That will eventually start to eat up memory. You should have been using on instead of click all along, but you can still do the same with vanilla JS..

Consider this re-write of your "DOMContentLoaded" function..

// Listen for a click on the DOM
document.addEventListener('click', function(e){

    // Determine if the click landed on your button (or whatever that is)
    if(!e.target.classList.contains("modal-overlay__close")) return;

    // Your button was clicked.. do whatever
    document.getElementById(applycouponPopupId).classList.remove("modal-overlay_visible");

});

This is 4 lines compared to your 8 and it only registers a single function in the event stack. Major memory improvement. And this code does not require the DOM to be loaded first.

Answer 3

Array.from() is not supported by Internet Explorer. If you don't care about supporting such browsers, then don't worry about, but if you're coding for a public website it's worth noting that IE still represents about 4% of the global browser market share at the time of this response.

Despite not being an array, the HtmlCollection object returned from document.getElementsByClassName will still have a length property, so consider instead using a normal while loop or for loop to access the set of elements.

For example, instead of this...

var closeElements = document.getElementsByClassName("js-modal-close");
Array.from(closeElements).forEach(function(element) {
    element.addEventListener('click', closeMethod);
});

you could do this...

var closeElements = document.getElementsByClassName("js-modal-close");
var i = closeElements.length;
while(i-- > 0){
    closeElements[i].addEventListener('click',closeMethod);
}