4
\$\begingroup\$

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.

\$\endgroup\$
7
  • 1
    \$\begingroup\$ We discourage updating your code after you've received an answer, since it can invalidate existing answers and make it difficult for new answerers to understand what's being asked. If you want, you can post a self-answer with your updated code, or you can post a new question with the updated code. See What should I do when someone answers my question? \$\endgroup\$
    – Thriggle
    Jul 11, 2017 at 20:41
  • \$\begingroup\$ judging by that humongous "DOMContentLoaded" block, I would say your senior dev is more of a css guy... \$\endgroup\$ Jul 11, 2017 at 20:44
  • \$\begingroup\$ @Thriggle, thank you for your suggestion. But i've left initial code, and updated one. Or it's still not the best practice? \$\endgroup\$
    – MployBy
    Jul 11, 2017 at 21:02
  • 1
    \$\begingroup\$ From the Help Center: "Do not add an improved version of the code after receiving an answer. Including revised versions of the code makes the question confusing, especially if someone later reviews the newer code." \$\endgroup\$
    – Thriggle
    Jul 11, 2017 at 21:07
  • 1
    \$\begingroup\$ You can add it as a self answer, and people can then use the comments and up/down votes to provide feedback on the quality of the revised code, or you can wait to revise the code until after you've received several answers and then post the revision as a new "question". I just wanted to give you a fair heads up so that if/when the mods revert your edits you're not too surprised (as I was when I posted my first code for review). \$\endgroup\$
    – Thriggle
    Jul 11, 2017 at 21:15

3 Answers 3

2
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ Gerrit0, thank you for your suggestions. They were very helpful. Could you please check the updated version of the code? Thank you. \$\endgroup\$
    – MployBy
    Jul 11, 2017 at 20:15
1
\$\begingroup\$

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.

\$\endgroup\$
1
  • \$\begingroup\$ As per your advice I've replaced document.addEventListener("DOMContentLoaded", function() { ... }); function with your function and it works. Thank you for your suggestion. I've up voted your reply, but will choose Gerrit0 as answer cause he pointed major suggestion which leaded us to updated code1, which was further improved by your suggestion. \$\endgroup\$
    – MployBy
    Jul 11, 2017 at 22:51
0
\$\begingroup\$

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);
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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