0

I'm playing around with pure JavaScript, so I created a small fade in/out object, to adjust images opacity onmouseover and onmouseout. Fading works fine when the mouseover and mouseout actions are precise:

  1. Start moving the cursor from the white background
  2. Hover over an image
  3. Hover back over the white background

The problem is, as soon as I start to move the mouse "naturally" from one image to another, the fading (or rather the script itself) freezes.

I'm not sure whether it's a animation-speed problem, or there's something I'm missing in the implementation.

If someone has the time to take a look, I would appreciate a peer check, so I can crack the issue and learn new stuff.

Here's a fiddle: http://jsfiddle.net/6bd3xepe/

Thanks!

lesssugar
  • 15,486
  • 18
  • 65
  • 115
  • Well the problem line as far as I can see is this 'if (element_opacity.toFixed(2) < 1) {' runs infinitely as you "naturally scroll" – Scott Mitchell Aug 09 '14 at 22:53
  • Yep, that's the failing part. Thanks a lot. – lesssugar Aug 09 '14 at 23:01
  • I believe the real failing part is that when you are trying to do this natural movement over the images the fading out is getting cancelled by the following fading in action so the fading out never occurs as well... – Scott Mitchell Aug 09 '14 at 23:05
  • Right. I created separate interval properties for fade in and fade out (updated fiddle: http://jsfiddle.net/6bd3xepe/5/). Works much better, however still high speed of the cursor movement can kill it. – lesssugar Aug 09 '14 at 23:14
  • Nice. I'd probably suggest try not using "that" as often as possible. There is definitely an easy way to avoid that. In this case not passing an anonymous function to set timeout and instead encapsulating that brighten/dimming in a function that you bind "this" too. – Scott Mitchell Aug 09 '14 at 23:22
  • Yeah. The design is borked in "that" way ;-P I've tried also making default_opacity an array (http://jsfiddle.net/6bd3xepe/6/) .. but it makes the behaviour even worse .. a redesign seems in order - just keep track of your objects & references properly ;-) – flowtron Aug 09 '14 at 23:24
  • ScottMitchell, flowtron - good feedback, guys. Surely the script needs refactoring. At least I know what to dig into now. Cheers :) – lesssugar Aug 09 '14 at 23:28

1 Answers1

1

As I see it, you have one INTERVAL for you FADER, you need one for each IMG. My jsfiddle fixes this. I added an ALT-attribute to each IMG with "dome" content, so as to circumvent the jsfiddle working on non-cat-images .. ignore that part - commented out below. There are some fundamental things wrong with the design - keeping track of objects & references is key. Usage of "this" & "that" aren't helping in the current implementation (see comments to OP). Also, on another note, the usage of "toFixed(2)" is not really required IMHO and you can shorten "o = o + 0.1" to "o += 0.1".

JS:

var fader = {

    target: document.getElementsByTagName('img'),
    interval: [],
    speed: 25,
    default_opacity: 1,

    init: function() {
        this.bindEvents();
    },

    // Get element's opacity and increase it up to 1
    fadeIn: function(element) {
        var element_opacity = this.getOpacity(element),
            that = this,
            idx = element.getAttribute('data-idx');
        console.log("fI: "+idx+" "+element_opacity);
        this.default_opacity = element_opacity.toFixed(2);

        this.interval[idx] = setInterval(function() {
            if (element_opacity.toFixed(2) < 1) {
                element_opacity = element_opacity + 0.1;
                element.style.opacity = element_opacity.toFixed(2);
            } else {
                clearInterval(that.interval[idx]);
            }
        }, that.speed);
    },

    // Get current opacity and decrease it back to the default one
    fadeOut: function(element) {
        var element_opacity = this.getOpacity(element),
            that = this,
            idx = element.getAttribute('data-idx');
        console.log("fO: "+idx+" "+element_opacity);
        this.interval[idx] = setInterval(function() {
            if (element_opacity.toFixed(2) > that.default_opacity) {
                element_opacity = element_opacity - 0.1;
                element.style.opacity = element_opacity.toFixed(2);
            } else {
                clearInterval(that.interval[idx]);
                element.removeAttribute('style');
            }
        }, that.speed);
    },

    // Get opacity of an element using computed styles
    getOpacity: function(element) {
        var styles = window.getComputedStyle(element),
            opacity = parseFloat(styles.getPropertyValue('opacity'));
        return opacity;
    },

    bindEvents: function() {
        var that = this, count = 0;
        for (var i in this.target) {   
            // the whole "dome" is just a fsfiddle hack - otherwise it sees 7 images instead of 4!
            //if( this.target[i].alt == "dome" ){
            console.log("COUNT: "+count);
            this.target[i].setAttribute('data-idx',count);
            this.target[i].onmouseover = function() {
                that.fadeIn(this);
            }
            this.target[i].onmouseout = function() {
                that.fadeOut(this);
            }
            count++;
            //}
        }
    }

};

fader.init();
flowtron
  • 854
  • 7
  • 20
  • That looks promising and works as intended. Of course, I guess there's no easy way to prevent fade in/out effect from breaking when someone starts to move the cursor crazily over the whole screen. Anyway, thanks! – lesssugar Aug 09 '14 at 23:22
  • I'm pretty sure this is caused by a confusion on "default_opacity" .. but my 2nd fiddle (http://jsfiddle.net/6bd3xepe/6/) is even crazier .. not frozen but almost seems like it is .. sorry, but it's bedtime for me here and so I'll leave the redesign up to you. – flowtron Aug 09 '14 at 23:31
  • True that. The `default_opacity` global was something I didn't like in the first place. – lesssugar Aug 09 '14 at 23:33