Removal of jQuery from script to auto-apply coupons from a link We are using a small extension that auto-applies coupons from a link.

The extension had pop-up JS based on jQuery:

&lt;script&gt;
    jQuery.noConflict();
    jQuery(function() {

        var appendthis = ("&lt;div class='modal-overlay js-modal-close'&gt;&lt;/div&gt;");

        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();

    });
&lt;/script&gt;


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:

&lt;script&gt;
    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);
&lt;/script&gt;


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.
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.