Increasing opacity based on an element's location

Question

I've got a piece of code that takes an element, checks where it is, and if it's beyond a set place in the viewport, make the opacity increase to 1. I've made the code so that it only runs the checks if they're needed, the problem is that the code looks atrocious, and I suspect there's a better way for me to be doing it.

var $wheresThisAt = viewportHeight*4;
        if (scrolled > ($wheresThisAt))
        {
            //there has to bve a better way...
            $('.salafoot').css('opacity', '0'); 
            $('#salamander-1').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+20)){
                $('#salamander-2').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+40)){
                $('#salamander-3').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+60)){
                $('#salamander-4').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+80)){
                $('#salamander-5').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+100)){
                $('#salamander-6').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+120)){
                $('#salamander-7').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+140)){
                $('#salamander-8').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+160)){
                $('#salamander-9').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+180)){
                $('#salamander-10').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+200)){
                $('#salamander-11').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+220)){
                $('#salamander-12').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+240)){
                $('#salamander-13').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+260)){
                $('#salamander-14').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+280)){
                $('#salamander-15').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+300)){
                $('#salamander-16').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+320)){
                $('#salamander-17').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+340)){
                $('#salamander-18').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+360)){
                $('#salamander-19').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+380)){
                $('#salamander-20').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+400)){
                $('#salamander-21').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+420)){
                $('#salamander-22').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+440)){
                $('#salamander-23').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+460)){
                $('#salamander-24').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+480)){
                $('#salamander-25').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+500)){
                $('#salamander-26').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+520)){
                $('#salamander-27').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+540)){
                $('#salamander-28').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+560)){
                $('#salamander-29').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+580)){
                $('#salamander-30').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+600)){
                $('#salamander-31').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+620)){
                $('#salamander-32').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+640)){
                $('#salamander-33').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+660)){
                $('#salamander-34').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+680)){
                $('#salamander-35').css('opacity', '1');
            if (scrolled > (($wheresThisAt)+700)){
                $('#salamander-36').css('opacity', '1');
            }}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}} // close all if statements before if (scrolled > ($wheresThisAt))
        } // end if (scrolled > ($wheresThisAt))

Here's a sample of the HTML that's being affected by the code

<div class="salafoot" id="salamander-8" style="opacity: 0;"></div>

As you can see I reuse almost identical code a lot of times, and if I want to change the number of steps (say to 37) I need to add another line. The if statements all get left open until the end, so that it only runs the check on the next line if it's useful (only checks +40 if the +20 has already been approved). All of the #salamander-(n) IDs are tied to the .salafoot class. The expected output is footprints that appear one at a time as you scroll down the page.

Kvothe · Accepted Answer · 2014-03-28 11:33:52Z

12

var top  = Math.max(Math.min(Math.ceil((scrolled-$wheresThisAt)/20),35),1); 

for (var i = 1; i < top; i++) {
    $('#salamander-' + i).css('opacity', '1');
}

This is similar to Amon's answer, but avoids the if structure.

edited Mar 28, 2014 at 11:33

answered Mar 28, 2014 at 11:28

Kvothe

3001 silver badge6 bronze badges

10

\$\begingroup\$ That long combination of Math.max, Math.min, Math.ceil on the same line I don't like. Otherwise a good approach. Using some additional variables would help make that long line more readable. \$\endgroup\$
– Simon Forsberg
Mar 28, 2014 at 13:08

Add a comment |

amon · Accepted Answer · 2014-03-28 12:30:37Z

17

Gah! Thanks for coming here.

Essentially, all your if-statements have the following structure:

if (scrolled > (($wheresThisAt)+(N * 20))){
    $('#salamander-(N + 1)').css('opacity', '1');

So we can comfortably roll that into a loop:

for (var i = 0; i < 35; i++) {
    if (scrolled <= ($wheresThisAt + i * 20)) {
        break;
    }
    $("#salamander-" + (i + 1)).css('opacity', '1');
}

edited Mar 28, 2014 at 12:30

answered Mar 28, 2014 at 11:25

amon

12.5k35 silver badges67 bronze badges

2

\$\begingroup\$ That's some terrible abuse of break and if, not to mention you can do it without an if structure. \$\endgroup\$
– Kvothe
Mar 28, 2014 at 11:43
4

\$\begingroup\$ @Kvothe: Isn't that a guard clause? blog.codinghorror.com/flattening-arrow-code \$\endgroup\$
– palacsint
Mar 28, 2014 at 12:03
4

\$\begingroup\$ @Kvothe (1) I see nothing wrong with break. While I'd personally prefer functional programming, JS is at its heart an imperative language. (2) I see no “terrible abuse” of anything in my code, except maybe magic numbers. (3) It was intended as a simple refactoring of the original code – I applied the rule “three or more: use a for”, and swapped > for <= with an “early return“, except that return is a break in the context of a loop. I decided not to use excessive cleverness which does make the if unnecessary. I don't quite understand your criticism. \$\endgroup\$
– amon
Mar 28, 2014 at 12:15
7

\$\begingroup\$ @Kvothe bad practice? I don't think so. Of course the alternative would be for (var i = 0; i < 35 && scrolled > ($wheresThisAt + i * 20); i++), but that's far less readable. The way I wrote it in my answer makes it clear that we are primarily iterating i over 0–35, and that we'll stop once we've scrolled too far. \$\endgroup\$
– amon
Mar 28, 2014 at 12:34
2

\$\begingroup\$ Passion abound this morning! Not abuse in my mind, but I would still have reversed the condition and put the .css() in the if block. Easier to grok that way. \$\endgroup\$
– konijn
Mar 28, 2014 at 14:44

| Show 3 more comments

cHao · Accepted Answer · 2014-03-28 12:07:25Z

0

You could easily loop over the element names...

for (var i = 2; i <= 36; ++i) {
    if (scrolled > ($wheresThisAt + (i-1)*20)) {
        $('#salamander-' + i).css('opacity', 1);
    }
    else break;
}

But if they're all defined sequentially in the page as i'm assuming they are, you could assign them a CSS class and then say something like

var shownCount = Math.max(Math.floor((scrolled - $wheresThisAt) / 20), 0) + 1;
$('.theirClass').slice(0, shownCount).css('opacity', 1);

and avoid the loop altogether (on your end, anyway).

edited Mar 28, 2014 at 12:07

answered Mar 28, 2014 at 11:42

cHao

3,15216 silver badges21 bronze badges

1

\$\begingroup\$ else break;, what...? Change the conditional of an if loop, break is just a glorified goto. \$\endgroup\$
– Kvothe
Mar 28, 2014 at 11:44
1

\$\begingroup\$ @Kvothe: Yeah. there's actually a bunch of nested if statements. :P It's not strictly necessary to skip to the end, but that's what the code it's replacing is basically doing. And there's no good reason to keep looping, or to go through the contortions required to satisfy the anti-break cargo cultists. \$\endgroup\$
– cHao
Mar 28, 2014 at 11:47
\$\begingroup\$ @cHao I'd call that premature optimization... this is only relevant when processing large sets of data, and ~40 items is definitely not a lot. \$\endgroup\$
– Vogel612
Mar 28, 2014 at 12:28
4

\$\begingroup\$ @Kvothe - break is a glorified GOTO, in the same way that a while loop, if statement, and for loop are all also glorified GOTO. What sets them apart is the target of the jump.... not the source. GOTO is ugly because the target of the jump is unconstrained. break is great because the target of the jump is a well-defined and optimized code-point. See what Knuth has to say about it, and realize that 'break' and 'continue' are the good kind of 'goto'. \$\endgroup\$
– rolfl
Mar 28, 2014 at 12:47
1

\$\begingroup\$ @Vogel612: I wasn't even talking about performance. If you know there won't be any more results, there's simply no good reason to continue. If you're continuing because you haven't realized that there's no further work to do, OK. But if you do realize it, and you're continuing solely to avoid a "glorified goto", that's boneheaded. \$\endgroup\$
– cHao
Mar 28, 2014 at 13:33

Add a comment |

SLuck49 · Accepted Answer · 2014-03-28 14:38:20Z

0

You're already getting all the elements to set the initially opacity: $('.salafoot').css('opacity', '0');

You may as well set the opacity for each item so you're only going over them once (this assumes they are in sequential order).

$('.salafoot').css('opacity', function(i, elem){
    return scrolled > $wheresThisAt+20*(1+i) ? '1' : '0';
});

answered Mar 28, 2014 at 14:38

SLuck49

101

Add a comment |

Tomas · Accepted Answer · 2014-03-28 15:50:00Z

0

I think @amon's and @Kvothe's solutions can be simplified a bit into something which is simple and readable at the same time:

var i = 0;
while (i < 35 && $wheresThisAt + i * 20 < scrolled) {
    $("#salamander-" + (i + 1)).css('opacity', '1');
    i++;
}

answered Mar 28, 2014 at 15:50

Tomas

101

Add a comment |

Stack Exchange Network

Increasing opacity based on an element's location

5 Answers 5

Your Answer

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

Hot Network Questions

Increasing opacity based on an element's location

5 Answers 5

Your Answer

Sign up or log in

Post as a guest

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

Related

Hot Network Questions

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