14
\$\begingroup\$

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

Please comment on things I can make better.

<!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>
\$\endgroup\$
3
  • 6
    \$\begingroup\$ You could skip all of this and let css transitions do the work for you. css3.bradshawenterprises.com/transitions \$\endgroup\$
    – Dagg
    Commented Dec 31, 2011 at 0:34
  • \$\begingroup\$ GGG, maybe it's a good idea. People will be updating there browsers more often in 2012. \$\endgroup\$
    – Hakan
    Commented Dec 31, 2011 at 10:33
  • 1
    \$\begingroup\$ 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. \$\endgroup\$
    – Dagg
    Commented Dec 31, 2011 at 17:54

2 Answers 2

Reset to default
6
\$\begingroup\$

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);
}
\$\endgroup\$
2
  • 2
    \$\begingroup\$ 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. \$\endgroup\$
    – gen_Eric
    Commented Dec 30, 2011 at 21:58
  • \$\begingroup\$ 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.) \$\endgroup\$
    – palacsint
    Commented Dec 30, 2011 at 22:08
4
\$\begingroup\$

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

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