1
\$\begingroup\$

I've created some custom code that will capture how long a user spent viewing a picture. The timer will stop when the user clicks or escapes from the fancybox view. Is there an alternate way for me to implement this functionality without using the counterStop setInterval() variable?

The specific reason why I added another setInterval() was so that I could quickly stop the first the counterIncrease variable. If the user clicked too fast and reopened the fancyBox, the timer would stack on itself and increment faster than the intended 1 second.

Also, as this was a refinement of a previous implementation of this exact functionality (lines of code was significantly reduced/refined), please point out, edit and critque any methods that you would have done differently, so long as the result is the same.

Credit and source of proof of concept goes to creator of this Fiddle - Šime Vidas.

HTML:

  <div id="fullBrochure">
    <div id="thumbWrapper">
      <a href="http://www.alexldixon.com/images/alexdixon.jpg" class="fancybox" target="_blank"><span class="innerText">Front Cover</span><div id="frontCover"><img src="http://www.alexldixon.com/images/alexdixon.jpg" /></div><span class="Zoom">ZOOM</span></a>
    </div>

    <div id="thumbWrapper">
      <a href="http://www.alexldixon.com/images/face.png"
      class="fancybox" target="_blank"><span class="innerText">Inside (fully
      opened)</span>

      <div id="frontCover"><img src="http://www.alexldixon.com/images/face.png" /></div><span class="Zoom">ZOOM</span>
      </a>
    </div>

    <div id="thumbWrapper">
      <a href="http://www.alexldixon.com/images/face2.png" class="fancybox" target="_blank"><span class="innerText">Inside Flap</span>

      <div id="frontCover"><img src="http://www.alexldixon.com/images/face2.png" /></div><span class="Zoom">ZOOM</span>
      </a>
    </div>

    <div id="thumbWrapper">
      <a href="http://www.alexldixon.com/images/face3.png" class="fancybox" target="_blank"><span class="innerText">Back Cover</span>

      <div id="frontCover"><img src="http://www.alexldixon.com/images/face3.png" /></div><span class="Zoom">ZOOM</span>
      </a>
    </div>
  </div>
<table summary="" class="mrQuestionTable" style="display: inline-block;">
    <tbody>
      <tr>
        <td id="Cell.0.0" class="mrGridCategoryText" style=  "text-Align: Left; vertical-align: Middle; background-color: #D8D8D8; width: 250px; border-color: black; border-width: 1px; border-left-style: Solid; border-right-style: Solid; border-top-style: Solid; border-bottom-style: Solid;">
        <span class="mrQuestionText" style="font-size: 8pt;">Front Cover:</span></td>

        <td id="Cell.1.0" style=  "text-Align: Center; vertical-align: Middle; background-color: #D8D8D8; width: 120px; border-color: black; border-width: 1px; border-left-style: Solid; border-right-style: Solid; border-top-style: Solid; border-bottom-style: Solid;">
        <span style=""><input type="text" name="_QPQ3_QAD__LP_Q__01_QQ3__Timer" id=  "_Q1_Q0_Q0" class="mrEdit" autocomplete="on" style="" maxlength="10" value=  "" /></span><span style=""><input type="checkbox" name=  "_QPQ3_QAD__LP_Q__01_QQ3__Timer_XNo__Ans" id="_Q1_Q0_Q0_X0" class="mrMultiple"
        style="" value="No__Ans" /> <label for="_Q1_Q0_Q0_X0"><span class=  "mrMultipleText" style=  "font-size: 8pt; text-Align: Center; vertical-align: Bottom; width: 120px; border-color: black; border-style: Solid; border-width: 1px;">
        No Answer</span></label></span></td>
      </tr>

      <tr>
        <td id="Cell.0.1" class="mrGridCategoryText" style=  "text-Align: Left; vertical-align: Middle; background-color: #F8F8F8; width: 250px; border-color: black; border-width: 1px; border-left-style: Solid; border-right-style: Solid; border-top-style: Solid; border-bottom-style: Solid;">
        <span class="mrQuestionText" style=" font-size: 8pt;">Inside (fully
        opened):</span></td>

        <td id="Cell.1.1" style=  "text-Align: Center; vertical-align: Middle; background-color: #F8F8F8; width: 120px; border-color: black; border-width: 1px; border-left-style: Solid; border-right-style: Solid; border-top-style: Solid; border-bottom-style: Solid;">
        <span style=""><input type="text" name="_QPQ3_QAD__LP_Q__02_QQ3__Timer" id=  "_Q1_Q1_Q0" class="mrEdit" autocomplete="on" style="" maxlength="10" value=  "" /></span><span style=""><input type="checkbox" name=  "_QPQ3_QAD__LP_Q__02_QQ3__Timer_XNo__Ans" id="_Q1_Q1_Q0_X0" class="mrMultiple"
        style="" value="No__Ans" /> <label for="_Q1_Q1_Q0_X0"><span class=  "mrMultipleText" style=  "font-size: 8pt; text-Align: Left; vertical-align: Middle; background-color: #F8F8F8; width: 250px; border-color: black; border-width: 1px; border-left-style: Solid; border-right-style: Solid; border-top-style: Solid; border-bottom-style: Solid;">
        No Answer</span></label></span></td>
      </tr>

      <tr>
        <td id="Cell.0.2" class="mrGridCategoryText" style=  "text-Align: Left; vertical-align: Middle; background-color: #D8D8D8; width: 250px; border-color: black; border-width: 1px; border-left-style: Solid; border-right-style: Solid; border-top-style: Solid; border-bottom-style: Solid;">
        <span class="mrQuestionText" style=" font-size: 8pt;">Inside flap:</span></td>

        <td id="Cell.1.2" style=  "text-Align: Center; vertical-align: Middle; background-color: #D8D8D8; width: 120px; border-color: black; border-width: 1px; border-left-style: Solid; border-right-style: Solid; border-top-style: Solid; border-bottom-style: Solid;">
        <span style=""><input type="text" name="_QPQ3_QAD__LP_Q__03_QQ3__Timer" id=  "_Q1_Q2_Q0" class="mrEdit" autocomplete="on" style="" maxlength="10" value=  "" /></span><span style=""><input type="checkbox" name=  "_QPQ3_QAD__LP_Q__03_QQ3__Timer_XNo__Ans" id="_Q1_Q2_Q0_X0" class="mrMultiple"
        style="" value="No__Ans" /> <label for="_Q1_Q2_Q0_X0"><span class=  "mrMultipleText" style=  "font-size: 8pt; text-Align: Center; vertical-align: Bottom; width: 120px; border-color: black; border-style: Solid; border-width: 1px;">
        No Answer</span></label></span></td>
      </tr>

      <tr>
        <td id="Cell.0.3" class="mrGridCategoryText" style=  "text-Align: Left; vertical-align: Middle; background-color: #F8F8F8; width: 250px; border-color: black; border-width: 1px; border-left-style: Solid; border-right-style: Solid; border-top-style: Solid; border-bottom-style: Solid;">
        <span class="mrQuestionText" style=" font-size: 8pt;">Back cover:</span></td>

        <td id="Cell.1.3" style=  "text-Align: Center; vertical-align: Middle; background-color: #F8F8F8; width: 120px; border-color: black; border-width: 1px; border-left-style: Solid; border-right-style: Solid; border-top-style: Solid; border-bottom-style: Solid;">
        <span style=""><input type="text" name="_QPQ3_QAD__LP_Q__04_QQ3__Timer" id=  "_Q1_Q3_Q0" class="mrEdit" autocomplete="on" style="" maxlength="10" value=  "" /></span><span style=""><input type="checkbox" name=  "_QPQ3_QAD__LP_Q__04_QQ3__Timer_XNo__Ans" id="_Q1_Q3_Q0_X0" class="mrMultiple"
        style="" value="No__Ans" /> <label for="_Q1_Q3_Q0_X0"><span class=  "mrMultipleText" style=  "font-size: 8pt; text-Align: Left; vertical-align: Middle; background-color: #F8F8F8; width: 250px; border-color: black; border-width: 1px; border-left-style: Solid; border-right-style: Solid; border-top-style: Solid; border-bottom-style: Solid;">
        No Answer</span></label></span></td>
      </tr>
    </tbody>
  </table>

JavaScript:

    $(".fancybox").fancybox({
        openEffect  : 'none',
        closeEffect : 'none',
        openSpeed : '0',
        closeSpeed : '0',
        arrows: false,
        mouseWheel: false
    });

    if(jQuery('div#IsInternetExplorer').length > 0)
    {
        jQuery('.innerText, .Zoom, a.innerText, a.Zoom, span.innerText, span.Zoom, #thumbWrapper, #frontCover, #insideOpen, #insideFlap, #backCover').css("cursor", "all-scroll");
    }

    //Start a timer on click for each of the 4 images.
    var TimerBox = jQuery('input[type=text]'), currentWrapper = $('div#fullBrochure > div#thumbWrapper'), timeArray = [1, 1, 1, 1];
    TimerBox.addClass('Timers');
    currentWrapper.find('a').attr("tabIndex", "-1");
    currentWrapper.keydown(function (z) {
        if (z.which == 13 || event.keyCode == 13) {
            z.preventDefault();
        }
    });

    function targetDiv(currentTimer) {
        var $currentTimer = $(currentTimer);
        $currentTimer.addClass('selected').siblings().removeClass('selected');                  
    }

    currentWrapper.on('click', function (c) {                           
        targetDiv(this);
        var $adIndex = $(this).index(), $timerIndex = TimerBox.filter(':eq(' + $adIndex + ')');
        if($(this).hasClass('selected')) {      
            var counterIncrease = setInterval(function() {          
                $timerIndex.val(timeArray[$adIndex]++);
                $timerIndex.addClass('counting');               
            }, 1000);

            var counterStop = setInterval(function() {
                if($('div.selected').length === 0) {
                    clearInterval(counterIncrease);
                    clearInterval(counterStop);
                } else {
                    $('div.fancybox-overlay:eq(0), a.fancybox-close:eq(0)').on('mouseup', function () {
                        currentWrapper.removeClass('selected');
                        $timerIndex.removeClass('counting');
                    });
                    $(document).keydown(function(esc){
                       var code = esc.keyCode ? esc.keyCode : esc.which;
                       if(code === 27) {
                        currentWrapper.removeClass('selected');
                        $timerIndex.removeClass('counting');
                       }
                    });
                }
            }, 100);
        }
    });

Working Example

\$\endgroup\$
  • \$\begingroup\$ Instead of setInterval could you get the current date/time when they open the fancybox, then the date/time when they close and calculate the difference? You could store the value in a data- attribute so that you could add to it if they click on the same item more than once. \$\endgroup\$ – Gary Storey Jul 29 '15 at 21:50
  • \$\begingroup\$ Hi Gary, this sounds like a solid concept and I see that storing numbers in data in a realize and common practice for jQuery programming. May you please provide an example with emphasis on date arithmetic and what steps to do, on event, to store and invoke it. \$\endgroup\$ – Alexander Dixon Jul 30 '15 at 14:19
  • \$\begingroup\$ I created a fiddle so you can see what I mean. \$\endgroup\$ – Gary Storey Jul 30 '15 at 14:50
  • \$\begingroup\$ I appreciate this Gary. I will definitely incorporate your methods into my practice. However, could you please guide me to a write-up or documention on what these lines do. (function ($, window, document, undefined) { ....and..... })(jQuery, window, document); \$\endgroup\$ – Alexander Dixon Jul 30 '15 at 15:59
  • \$\begingroup\$ Sure, its an IIFE(Immediately Invoked Function Expression). It basically creates a closure (also explained by the article above) which gives you your own (effectively private) scope. In this format the last line passes those objects into your function. The lines at the top is the "internal" names for those objects. So $ = jQuery and window=window, etc. undefined is not passed so it's value is undefined (only needed for old pre-ES5 browsers). \$\endgroup\$ – Gary Storey Jul 30 '15 at 21:29
1
\$\begingroup\$

For most of your code, you use the $ sign for jQuery, like here:

$(".fancybox").fancybox({...});

However, there are two places where you do not use the $. Instead, you use the name jQuery. Here is one of the spots:

if(jQuery('div#IsInternetExplorer').length > 0)

Why did you switch? You should be consistent with which function you use, as it is good practice.


You are inconsistent with your brace types:

if(jQuery('div#IsInternetExplorer').length > 0)
{ // <=======
    ...
} // <=========

but here you do:

function targetDiv(currentTimer) { // <======
    ...                  
} // <==============

Generally, the style for JavaScript is to go with the second version. However, both work equally as well, but you should choose one and stick with that one.


You have a few magic numbers in your code, especially around the areas where you are checking key codes:

if (z.which == 13 || event.keyCode == 13) {

and:

if(code === 27) {

What do these numbers mean? Don't make the person who is reading your code (or yourself, possibly in the future) have to do some google searches (or tests) to find out what these numbers mean; create constants.

At the top of your code, create a constant variable for each magic number like this:

var CONSTANT_NAME = magic_number;

Then, when you need to use that number again, just use the variable.


Another inconsistency:

$('div.fancybox-overlay:eq(0), a.fancybox-close:eq(0)').on('mouseup', ...);

and:

$(document).keydown(...);

Why are you setting an event in the first one using $.on, but in the second using that event's specific method? Again, you need to be consistent in your code.


I see why you need to have something to stop the counter as soon as the user is done viewing the image/video. However, I don't see why you needed to do that at an interval.

Let's break down what you are doing in this interval:

  • If the user is no longer watching the video, stop the intervals.

  • If the user is still watching the video, set event listeners for when the user presses a button.

There are a few problems with this: why are you setting the event listeners every single time the interval loops? There is absolutely nothing changing in the event listeners' functions.

A better way to go about this would be to set the event listeners outside of any interval. Don't worry; they will still run just fine when the event fires.

Now let's take this one step farther: both these events fire when someone is attempting to exit out of viewing and image/video.

My question is why are you using an interval to check if the user has exited the image/video, only then to stop the timer? Why don't you just stop the timer when the user exists the image/video?

Add this line to the appropriate spot in both of these functions:

clearInterval(counterIncrease);

Now that the second interval has been basically stripped down to nothing, you can completely get rid of this (hooray!).


var counterIncrease = setInterval(function() {          
    $timerIndex.val(timeArray[$adIndex]++);
    $timerIndex.addClass('counting');               
}, 1000);

Why are you addClassing $timerIndex every second? Once it's counting, it's counting and you don't need to say that every single second; you need only to specify it once, and then to remove it when it is no longer counting.

Remove this line and put it above the interval:

$timerIndex.addClass('counting');

You are inconsistent with your indentation. Some places you have a space of indentation, and others you have 4 spaces / 1 tab. Choose one. (Preferably 4 spaces or 1 tab).


\$\endgroup\$
  • \$\begingroup\$ The reason for the second setInterval() counterStop function is because the fancybox has a fadeOut effect of which I want to accurately account for (fancybox container div will still be visible by the DOM for a few milliseconds), down to the 100 milliseconds. By doing this, I avoid a glitch where the main counterIncrease interval does not properly clear/stop and can stack upon itself if a user 'rapidly clicks' on the same image. $(document).keydown was used because the .on() method did not bubble into the fancybox container, but I do not know why. I have made all of the other edits. \$\endgroup\$ – Alexander Dixon Jul 30 '15 at 2:32

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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