Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I decided to build my own fade in fade out function, since that is all I need on my page.

Here it is. Please comment on things I can make better!

NEW VERSION:

var done = true,
    fading_div = document.getElementById('fading_div'),
    fade_in_button = document.getElementById('fade_in'),
    fade_out_button = document.getElementById('fade_out');

function function_opacity(opacity_value) {
    fading_div.style.opacity = opacity_value / 100;
    fading_div.style.filter = 'alpha(opacity=' + opacity_value + ')';
}

function function_fade_out(opacity_value) {
    function_opacity(opacity_value);
    if (opacity_value == 1) {
        fading_div.style.display = 'none';
        done = true;
    }
}

function function_fade_in(opacity_value) {
    function_opacity(opacity_value);
    if (opacity_value == 1) {
        fading_div.style.display = 'block';
    }
    if (opacity_value == 100) {
        done = true;
    }
}
// fade in button
fade_in_button.onclick = function () {
    if (done && fading_div.style.opacity !== '1') {
        done = false;
        for (var i = 1; i <= 100; i++) {
            setTimeout((function (x) {
                return function () {
                    function_fade_in(x)
                };
            })(i), i * 10);
        }
    }
};
// fade out button
fade_out_button.onclick = function () {
    if (done && fading_div.style.opacity !== '0') {
        done = false;
        for (var i = 1; i <= 100; i++) {
            setTimeout((function (x) {
                return function () {
                    function_fade_out(x)
                };
            })(100 - i), i * 10);
        }
    }
};

OLD VERSION:

<!DOCTYPE html>
<html>
<head>
<title></title>
<meta charset="utf-8" />
</head>

<body>
        <div>
        <span id="fade_in">Fade In</span> | 
        <span id="fade_out">Fade Out</span></div>
        <div id="fading_div" style="display:none;height:100px;background:#f00">Fading Box</div>
    </div>
</div>

<script type="text/javascript">
// global varibles
var done = true,
    fading_div = document.getElementById('fading_div'),
    fade_in_button = document.getElementById('fade_in'),
    fade_out_button = document.getElementById('fade_out');

function function_opacity(opacity_value, fade_in_or_fade_out) { // fade_in_or_out - 0 = fade in, 1 = fade out
    fading_div.style.opacity = opacity_value / 100;
    fading_div.style.filter = 'alpha(opacity=' + opacity_value + ')';
    if (fade_in_or_fade_out == 'in' && opacity_value == 1) {
        fading_div.style.display = 'block';
    }
    if (fade_in_or_fade_out == 'in' && opacity_value == 100) {
        done = true;
    }
    if (fade_in_or_fade_out == 'out' && opacity_value == 1) {
        fading_div.style.display = 'none';
        done = true;
    }
}

// fade in button
fade_in_button.onclick = function () {
    if (done && fading_div.style.opacity !== '1') {
        done = false;
        for (var i = 1; i <= 100; i++) {
            setTimeout("function_opacity(" + i + ",'in')", i * 5);
        }
    }
};

// fade out button
fade_out_button.onclick = function () {
    if (done && fading_div.style.opacity !== '0') {
        done = false;
        for (var i = 100; i >= 1; i--) {
            setTimeout("function_opacity(" + i + ",'out')", (i - 100) * -1 * 5);
        }
    }
};
alert (test);
</script>
</body>
</html>
share|improve this question
4  
You could skip all of this and let css transitions do the work for you. css3.bradshawenterprises.com/transitions –  Dagg Dec 31 '11 at 0:34
    
GGG, maybe it's a good idea. People will be updating there browsers more often in 2012. –  Hakan Dec 31 '11 at 10:33
1  
Yeah, I think it's fine for something like a fade (progressive enhancement). People who's browsers don't support it won't see the fade, but the thing will still appear/disappear, so they're just missing out on a visual effect, and you get to save 50 or 60 lines of javascript. –  Dagg Dec 31 '11 at 17:54

3 Answers 3

up vote 4 down vote accepted

Just some generic notes about the JavaScript code: I'd extract out a setOpacity function and create a fadeOut and a fadeIn function too.

function setOpacity(opacity) {
    fading_div.style.opacity = opacity / 100;
    fading_div.style.filter = 'alpha(opacity=' + opacity + ')';
}

function fadeOut(opacity) {
    setOpacity(opacity);
    if (opacity == 1) {
        fading_div.style.display = 'none';
        done = true;
    }
}

function fadeIn(opacity) {
    setOpacity(opacity);
    if (opacity == 1) {
        fading_div.style.display = 'block';
    }
    if (opacity == 100) {
        done = true;
    }
}

...
setTimeout("fadeIn(" + i + ")", i * 5);
...

It eliminates the in and out magic constants and lots of conditions which checks their values.

Using variable name suffixes like opacity_value looks a little bit redundant (since variables stores values), so I've renamed them. The same is true for the function_opacity function.

I've changed the second for loop too to use fewer arithmetic operations, I think the following is easier to read:

for (var i = 1; i <= 100; i++) {
    setTimeout("fadeOut(" + (100 - i) + ")", i * 5);
}
share|improve this answer
1  
Please don't pass strings to setTimeout! It evals them. Pass a function. setTimeout((function(x){ return function(){ fadeOut(100-x); }; })(i), i * 5); It's in a closure because i changes in the for loop. –  Rocket Hazmat Dec 30 '11 at 21:58
    
Thanks, @Rocket! Could you give me an example or link with some explanation why is it better? I'm not a JavaScript guru. (Maybe you want to write it as an answer since in the question there is the same string passing, and we will be able to upvote it.) –  palacsint Dec 30 '11 at 22:08

This is just an extension to @palacsint's answer. You shouldn't pass strings to setTimeout, it uses eval, which is inefficient and insecure. You should pass a function.

Problem is, in the for loop i changes, so you'll have to use a closure.

Don't do this:

for (var i = 100; i >= 1; i--) {
   setTimeout("function_opacity(" + i + ",'out')", (i - 100) * -1 * 5);
}

Instead do:

for (var i = 100; i >= 1; i--) {
   setTimeout((function(x){
      return function(){
         function_opacity(x, 'out')
      };
   })(i), (i - 100) * -1 * 5);
}

This may look a little messy. I suggest declaring a function that returns a function separately.

function call_opacity(i, d){
    return function(){
      function_opacity(i, d);
    };
}

Then do:

for (var i = 100; i >= 1; i--) {
   setTimeout(call_opacity(i, 'out'), (i - 100) * -1 * 5);
}
share|improve this answer
1  
Thanks Rocket! Here is a link on the subject to back up your answer. Thanks for shring! stackoverflow.com/questions/6232574/… –  Hakan Dec 31 '11 at 9:14

I used OPACITY to make it show/hide. See this example, full code (without jQuery):

<a href="javascript:ShowDiv('MyMesage');"> Click here</a>

<div id="MyMesage" style="display:none; background-color:pink; margin:0 0 0 100px;width:200px;">
blablabla
</div>

<script>
function ShowDiv(name){
    //duration of transition (1000 miliseconds equals 1 second)
    var duration = 1000;
    // how many times should it should be changed in delay duration
    var AmountOfActions=100;

    var diiv= document.getElementById(name);
    diiv.style.opacity = '0';   diiv.style.display = 'block';   var counte=0;
    setInterval(function(){counte ++;
       if ( counte<AmountOfActions) { diiv.style.opacity = counte/AmountOfActions;}
    },
    duration / AmountOfActions);
}
</script>
share|improve this answer
5  
Welcome! On Code Review, we expect answers to give some kind of feedback for the original code. You may keep this code snippet, but we recommend you explain how it may help the OP. –  Jamal Jan 22 at 15:33

protected by Mat's Mug Jan 22 at 15:46

Thank you for your interest in this question. Because it has attracted low-quality answers, posting an answer now requires 10 reputation on this site.

Would you like to answer one of these unanswered questions instead?

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