3
\$\begingroup\$

I'm playing around with prototypes, constructor functions and javascript - I'm not really a Javascript developer but am interested in this.

Am I on the right track with this code? I'm looking for a code review from a JS developer. I know the codes a bit funky.

I created an example of it working here.

function modal( width, height, backgroundColor, overlayColor, showOverlay, url )
{
    this.width = width; // a percentage based number %
    this.height = height; // optional, usually needs not to be set
    this.backgroundColor = backgroundColor; // set the background colour to the modal
    this.overlayColor = overlayColor; // as hex, to be converted to rgba
    this.url = url;
    this.showOverlay = showOverlay;  
    this.createModal = function()
    {
        // create elements to add classes to and insert in to dom
        var modalOverlay = document.createElement('div')
        ,   modalContent = document.createElement('div')
        ,   modalInner = document.createElement('div');

        // build html structure
        modalOverlay.className = "modalOverlay";
        modalContent.className = "modalContent";
        modalOverlay.appendChild(modalContent);
        modalInner.className = "modalInner";
        modalContent.appendChild(modalInner);

        // add new html structure to body
        document.body.appendChild(modalOverlay);

        // Theme model
        this.themeModel();
    }
    this.themeModel = function() {

        // is there a background color?
        var overlayBgColor = this.showOverlay ? this.convertHext(this.overlayColor,50) : "transparent";

        $('.modalOverlay').css({ 
            'background-color': overlayBgColor 
        });

        $('.modalContent').css({
            'background-color': this.backgroundColor,
            'width': this.width,
            'height': this.height
        });

        // load content
        this.loadContent();
    }
    this.loadContent = function(){
        if( url )
        {
            $('.modalInner').load(url);
        }
        else
        {
            $('.modalInner').html("please provide a url to load! <a class='close-modal' href=''>close</a>");
        }

    }
    this.showModal = function(){
        this.createModal();
        $('.modalOverlay').fadeIn(50);
        $('body').addClass('modalShow');
    }
    this.hideModal = function(){ 
        $('.modalOverlay').fadeOut(300, function(){
           $('.modalOverlay').remove();
        }) 
        $('body').removeClass('modalShow');
    }
    this.convertHext = function(hex,opacity)
    {
        hex = hex.replace('#','');
        r = parseInt(hex.substring(0,2), 16);
        g = parseInt(hex.substring(2,4), 16);
        b = parseInt(hex.substring(4,6), 16);
        result = 'rgba('+r+','+g+','+b+','+opacity/100+')';
        return result;
    }
}
body 
{
    position: relative;
    margin: 0; /* reset margin */
    padding: 0; /* reset padding */
}

.modalShow .modalContent
{
    opacity: 1;
   overflow':'hidden';
  -webkit-transition: all .3s ease-out; 
          transition: all .3s ease-out; 
margin-top: 30px;
}

.modalOverlay
{
    display: none;
    overflow: auto;
    overflow-y: scroll;
    position: fixed;
    top: 0;
    right: 0;
    bottom: 0;
    left: 0;
    z-index: 9999;
    -webkit-overflow-scrolling: touch;
    outline: 0;
    background-color: #2d2d2d;
    background-color: rgba(45,45,45,0.2);

}
.modalContent 
{
    width: 50%;
    height: auto;
    background-color: #FFFFFF;

    margin-top: 0;
    margin-bottom: 30px;
    margin-left: auto;
    margin-right: auto;
    opacity: 0;
  -webkit-transition: all .3s ease-out; 
          transition: all .3s ease-out; 
}

.modalInner
{
    padding: 20px;
}
<body>

    <a class="showModal" href="">show modal</a>

</body>

Improve this question
\$\endgroup\$

3 Answers 3

Reset to default
5
\$\begingroup\$

Here's some thoughts

function modal(width, height, backgroundColor, overlayColor, showOverlay, url) {

One problem some programmers rant with this is that if you forget a parameter, or forget the order. This would seriously impale your code. An alternative is to pass an object that contains the config.

new Modal({
  width : 100,
  height : 100
});

One advantage of this over the previous is that you become more verbose and don't need to remember the order of the params. Additionally, you can use $.extend to place in defaults, if they didn't exist on options.

function Modal(options){
  options = $.extend(options,{
    width : /*default width */,
    height : /*default height */
  });
  ...
}

this.createModal = function () {...};
this.themeModel = function () {...};

You lose the advantage of prototypal inheritance. Better put methods on the prototype so they are shared. Advantage is you save memory by not duplicating functions per instance. Disadvantage is you lose emulated private property - but everything is accessible one way or another, no point using privates.

$('.modalOverlay').css({
    'background-color': overlayBgColor
});

$('.modalContent').css({
  'background-color': this.backgroundColor,
  'width': this.width,
  'height': this.height
});

Suggesting you move this to CSS by adding/removing classes. That way, proper separation of concerns is attained. You don't want to be debugging and 1 hour later, realize that the styles were not in the CSS files but in JS files.

To clarify, this kind of code will bite back in the long run. Suggesting you add a class to the top-most element of your modal HTML, and then style relative to it. You can provide a custom class name to the constructor instead of the styles.

.blackish-modal a{/*style for links on modal with blackish-modal class*/}
.whitish-modal a{/*same goes, but for whitish-modal class*/}

$('.modalInner').load(url);

IMO, $.fn.load is just a convenience function. Suggesting you use the lower-level AJAX functions for more flexibility. But as a convenience function, if it works for you, that's fine.

result = 'rgba(' + r + ',' + g + ',' + b + ',' + opacity / 100 + ')';

String cocatenation in JS is really messy with all that quotes. Here's an alternative:

var rgba = [r,g,b,opacity/100].join(',');
result = 'rgba(' + rgba + ')';

var delegateLoginModal = new modal("90%", "", "", "", true, "");

// create your events
$('body').on('click', '.showModal', function (e) {
    e.preventDefault();
    delegateLoginModal.showModal();
});

Suggesting you look at how Bootstrap implemented their modal. It's in a form of a jQuery plugin. Your code looks cleaner if it did something as clean as:

$('someElement').modal(/*config|method*/);
Improve this answer
\$\endgroup\$
2
  • \$\begingroup\$ Great feedback, this is enough to get me reading and looking over the code. One bit I didn't get though is toggling classes as the point of the code was to be able to change the colours.. you would lose this with the idea of toggling classes. \$\endgroup\$
    – Iamsamstimpson
    Nov 18, 2014 at 14:53
  • \$\begingroup\$ @pixeltooth Yup, it does seem that I misunderstood the code. However, this kind of code will bite back in the long run. Still suggesting you use a class, and style around that class instead. \$\endgroup\$
    – Joseph
    Nov 18, 2014 at 15:52
5
\$\begingroup\$

So, I'm not an expert JS programmer but there are a couple of things that can be improved.

Naming

I would encourage you to declare and name classes in the following way:

var ClassName = function(arguments) {...}

Having a capital letter for the class name helps it to stand out against standard functions and allows you to quickly identify your own code.

Your methods should have names that better describe what they do. A better name for themeModal could be applyTheme as it more accurately and clearly describes what the method does.

Scope

A wonderful practice I have found when writing JavaScript like this, is to get a concrete handle on this so there is no ambiguity. In a class, I like to have a line like:

var ClassName = function(obj) {
    var self = this; // Concrete 'this' handle
    self.classProperty = obj.property;
    self.someMethod = function() {
        this.test = "test"; // this refers to the function
        self.test = "not a test"; // self refers to the class "scope"
    };
}

This way the code is easier to read and you avoid some nasty side-effects.

Method Chaining

While not really bad, I would consider a structure like this:

createModal = function() {
    buildElements();
    applyTheme();
    loadContent();
}

Instead of calling each method from the next, encapsulate them in a single method. This way those methods have fewer side-effects. This is more of a design decision and may not be correct in this case, but consider some of these questions:

  • What if I want to call ONLY the applyTheme (to change the color)?
  • What if I need to keep the theme and elements but change the content?
  • What if as part of a pre-load process I create the DOM elements, but have to wait for user input to apply styling?

With a structure as I listed above, all of these things are possible by calling either each method only, or calling the createModal method (but they are not possible if the methods are chained together).

Arguments

It is much easier to make the class flexible and cleaner if you include a generic set of options as arguments (a JS object) that can be modified and passed in.

var Model = function(options) {
    var self = this;

    self.width = options.width;
    self.height = options.height;
    // etc...
}

As a final note, I renamed your delegation variable name to loginModal and I made some of the functions in your class "private" (JS doesn't really have private scope, I just made it so they are not exposed outside of the closure) so they cannot be called externally. It seemed to make sense to me, but you may see it differently.

So, here is the fork I made of your code:

var Modal = function (options) {
    var self = this; // concrete this reference for instance members

    self.width = 0;
    self.height = 0;
    self.backgroundColor = "";
    self.overlayColor = "";
    self.url = "";
    self.showOverlay = false;

    // Fill properties based on the options
    if (typeof options != 'undefined' && options != undefined && options != null) {
        self.width = options.width; // a percentage based number %
        self.height = options.height; // optional, usually needs not to be set
        self.backgroundColor = options.backgroundColor; // set the background colour to the modal
        self.overlayColor = options.overlayColor; // as hex, to be converted to rgba
        self.url = options.url;
        self.showOverlay = options.showOverlay;
    }

    var buildModal = function () {
        var modalOverlay = document.createElement('div'),
            modalContent = document.createElement('div'),
            modalInner = document.createElement('div');

        // build html structure
        modalOverlay.className = "modalOverlay";
        modalContent.className = "modalContent";
        modalOverlay.appendChild(modalContent);
        modalInner.className = "modalInner";
        modalContent.appendChild(modalInner);

        // add new html structure to body
        document.body.appendChild(modalOverlay);
    };

    self.createModal = function () {
        // create elements to add classes to and insert in to dom
        buildModal();
        // apply theme
        self.applyTheme();
        // load content
        self.loadContent();
    }

    self.applyTheme = function () {

        // is there a background color?
        var overlayBgColor = self.showOverlay ? convertHex(self.overlayColor, 50) : "transparent";

        $('.modalOverlay').css({
            'background-color': overlayBgColor
        });

        $('.modalContent').css({
            'background-color': self.backgroundColor,
                'width': self.width,
                'height': self.height
        });


    }
    self.loadContent = function () {
        if (self.url) {
            $('.modalInner').load(self.url);
        } else {
            $('.modalInner').html("please provide a url to load! <a class='close-modal' href=''>close</a>");
        }

    }
    self.showModal = function () {
        self.createModal();
        $('.modalOverlay').fadeIn(50);
        $('body').addClass('modalShow');
    }
    self.hideModal = function () {
        $('.modalOverlay').fadeOut(300, function () {
            $(this).remove();
        })
        $('body').removeClass('modalShow');
    }
    var convertHex = function (hex, opacity) {
        hex = hex.replace('#', '');
        r = parseInt(hex.substring(0, 2), 16);
        g = parseInt(hex.substring(2, 4), 16);
        b = parseInt(hex.substring(4, 6), 16);
        result = 'rgba(' + r + ',' + g + ',' + b + ',' + opacity / 100 + ')';
        return result;
    }
}

//create a instance of the modal on page load
// width, height, modal window colour, background-color, overlay visible?, url to load content.
var loginModal = new Modal({
    width: "90%",
    height: "",
    backgroundColor: "#cccccc",
    overlayColor: "#88dd88",
    showOverlay: true,
    url: ""
});

You can try it here: http://jsfiddle.net/xDaevax/97rpcbv3/

Improve this answer
\$\endgroup\$
-2
\$\begingroup\$

Your code is more or less good.

this.hideModal = function(){ 
    $('.modalOverlay').fadeOut(300, function(){
       $('.modalOverlay').remove();

This part I don't think is right, try replacing it with

.fadeOut(300,

with maybe 

.fadeOut(145,

I find that this works better.

Improve this answer
\$\endgroup\$
3
  • \$\begingroup\$ Do you think you could expand on why you don't think it's right? \$\endgroup\$
    – Nick Udell
    Nov 18, 2014 at 14:16
  • 1
    \$\begingroup\$ Welcome to CR @ThatGuy. We try to encourage answers that review the code; checking whether the code is idiomatic, readable, maintainable, correct, secure. You can review 1 or more of these aspects. Reviewing the fadeOut timing is not part of that, hence the minus 2. (Twasnt me) \$\endgroup\$
    – konijn
    Nov 18, 2014 at 15:05
  • \$\begingroup\$ @ThatGuy Welcome to Code Review! It appears your answer is not very clear on why you feel it is more or less good, or what the reason is behind your suggestion. I would encourage you to go over How do I write a good answer in the Help Center. If you want to improve your answer's score please improve it and we will be happy to upvote your good answers! \$\endgroup\$
    – Phrancis
    Nov 18, 2014 at 15:09

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.