1
\$\begingroup\$

Toggling images in a table. The example would be a user using a checklist program. She can click on a cell next to task to toggle available status images. Like a checkmark, fail, question.

Other design suggestions are welcomed

<!DOCTYPE html>
<html>
<head>
    <title>Test 123</title>
    <style type="text/css">
        body {
            background-color: teal;
        }
    </style>
    <script type="text/javascript" src="https://code.jquery.com/jquery-3.1.0.js"></script>
</head>
<body>
    <table border="1">
        <tr>
            <td><img id="1" class="imgX" src="pic0.png" alt="No Pic" /></td>
            <td><img id="2" class="imgX" src="pic0.png" alt="No Pic" /></td>
        </tr>
        <tr>
            <td><img id="3" class="imgX" src="pic0.png" alt="No Pic" /></td>
            <td><img id="4" class="imgX" src="pic0.png" alt="No Pic" /></td>
        </tr>
    </table>

    <script type="text/javascript">
        $(function () {
            var lastImageIdClicked = "";
            var timer;

            $('.imgX').on('click', function () {
                el = $(this);
                elId = el.attr('id');
                elSrc = el.attr('src');
                var clicks = 0;

                if (elId === lastImageIdClicked) {
                    clicks++;
                    clearTimeout(timer);
                }
                else {
                    clicks = 0;
                }

                var fileName = elSrc;
                lastImageIdClicked = elId;
                var re = /(pic)(.*)(.png)/;
                var imgNumber = fileName.match(re)[2]; //todo: why array
                clicks = Number(imgNumber) + 1;

                if (clicks > 3) clicks = 0;

                var eleImage = $(this).attr('src', 'pic{0}.png'.replace('{0}', clicks));

                //Save image if 5 seconds have passed
                timer = setTimeout(function () {
                    alert('saved');
                }, 5000);
            });
        });
    </script>
</body>
</html>
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Here's demo of your code. As far as I understand, you want to show next image(0 -> 1, 1-> 2 and 2 -> 0) when clicked. This can be done with following code $('.imgX').on('click', function() { $(this).attr('src', function(i, src) { return src.replace(/(\d+)\.png$/i, function(m, $1) { return ((Number($1) + 1) % 3) + '.png'; }); }); }); Note that this code doesn't contain logic of save-the timeout. \$\endgroup\$ – Tushar Feb 10 '17 at 3:15
  • \$\begingroup\$ Will there be more than three states? How exactly is the saving process/timer supposed to work? Do you have form fields representing the current state of each item as well or how do you store the state to be saved later? \$\endgroup\$ – insertusernamehere Feb 10 '17 at 7:54
  • \$\begingroup\$ It'd be nice if number of states is configurable. The idea with save process/timer is I don't want it to save immediately each time an image is toggled. Because if I'm just toggling to get to last image there's no need to save images that I'm cycling through. \$\endgroup\$ – Rod Feb 10 '17 at 15:09
  • \$\begingroup\$ So if there's no activity for 5 seconds in a cell that has changed then save to database. \$\endgroup\$ – Rod Feb 10 '17 at 15:11
3
\$\begingroup\$

You use a lot of jQuery, where plain javascript will suffice and is actually more efficient. A small example: el.attr('id') loops through the available attributes until it finds ID1. you can simply do el[0].id and select it without anything complex. The same fors for .src and some other values. I will add comment to the coding below to guide you through it:

1 This isnt 100% true, they've improved but you get the gist of it

Round 1:

$(function () {
    var lastImageIdClicked = "";
    var timer;

    $('.imgX').on('click', function () {
        // Was: el = $(this);
        // - Placed [var] before it to make it local scope
        // - We dont yet need it als a jQuery element
        // - el is very short and not descriptive. [elem] is more common:
        var elem = this ;
        // Was: el.attr('id');
        // - update to [elem] -> elem.id
        // - removed variable because we no longer use it (see a few lines below)
        //~ elId =  elem.id 
        // Was: el.attr('src');
        // - update to [elem] -> elem.id
        // - removed variable because we no longer use it (see a few lines below)
        //~ elSrc = elem.scr
        var clicks = 0;

        // Was: if (elId === lastImageIdClicked) {
        // - just use elem.id here. It's not more complex and actually tells me more
        if (elem.id === lastImageIdClicked) {
            clicks++;
            clearTimeout(timer);
        }
        // This whole else is irrelevant as you set [var clicks = 0;] a few lines higher
        // else {
        //      clicks = 0;
        //}

        // Was: var fileName = elSrc;
        // - Simply use elem.src
        // We use this only once a few lines below, just use [elem.src] there, no need to use a variable for that

        // Was: lastImageIdClicked = elId;
        // - Simply use elem.id
        lastImageIdClicked = elem.id;

        var re = /(pic)(.*)(.png)/;
        var imgNumber = elem.src.match(re)[2]; // todo: why array
        clicks = Number(imgNumber) + 1;

        // Was: if (clicks > 3) clicks = 0;
        // IMO you should _always_ use brackets: if (clicks > 3){ clicks = 0;}
        if (clicks > 3){ clicks = 0; }

        // Was: var eleImage = $(this).attr('src', 'pic{0}.png'.replace('{0}', clicks));
        // - we now just use this.src = /* ... */
        // - You place {0} in your string, then replace it? Just concat it directly: ['pic'+click+'.png']
        var eleImage = this.src = 'pic'+clicks+'.png';

        //Save image if 5 seconds have passed
        timer = setTimeout(function () {
            alert('saved');
        }, 5000);
    });
});

Clean that up:

$(function () {
    var lastImageIdClicked = "",
        timer;

        $('.imgX').on('click', function () {
            var elem = this ;
            var clicks = 0;

            if (elem.id === lastImageIdClicked) {
                clicks++;
                clearTimeout(timer);
            }
            lastImageIdClicked = elem.id;

            var re = /(pic)(.*)(.png)/;
            var imgNumber = elem.src.match(re)[2]; // todo: why array
            clicks = Number(imgNumber) + 1;

            if (clicks > 3){ clicks = 0; }

            var eleImage = this.src = 'pic'+clicks+'.png';

            //Save image if 5 seconds have passed
            timer = setTimeout(function () {
                alert('saved');
            }, 5000);
    });
});

Round 2

$(function () {
    var lastImageIdClicked = "",
        timer;

        $('.imgX').on('click', function () {
            var elem = this ;
            var clicks = 0;

            if (elem.id === lastImageIdClicked) {
                clicks++;
                clearTimeout(timer);
            }
            lastImageIdClicked = elem.id;

            // Was: var re = /(pic)(.*)(.png)/;
            // Was: var imgNumber = elem.src.match(re)[2]; // todo: why array
            // - This is a simple regex, no need to use a variable for it if you only use it once
            // - you select multiple groups now, you don't want that, only the number
            var imgNumber = elem.src.match(/pic(.*)\.png/)[1]; // todo: why array
            clicks = Number(imgNumber) + 1;

            if (clicks > 3){ clicks = 0; }

            // Was: var eleImage = this.src = 'pic'+clicks+'.png';
            // - Seemingly does nothing, assuming you want to change the src
            this.src = 'pic'+clicks+'.png';

            //Save image if 5 seconds have passed
            timer = setTimeout(function () {
                alert('saved');
            }, 5000);
    });
});

Clean that up

$(function () {
    var lastImageIdClicked = "",
        timer;

        $('.imgX').on('click', function () {
            var elem = this ;
            var clicks = 0;

            if (elem.id === lastImageIdClicked) {
                clicks++;
                clearTimeout(timer);
            }
            lastImageIdClicked = elem.id;

            var imgNumber = elem.src.match(/pic(.*)\.png/)[1]; // todo: why array
            clicks = Number(imgNumber) + 1;

            if (clicks > 3){ clicks = 0; }
            this.src = 'pic'+clicks+'.png';

            //Save image if 5 seconds have passed
            timer = setTimeout(function () {
                alert('saved');
            }, 5000);
    });
});

Round 3

This is for you do to yourself:
- Instead if hardcoding max-images = 3, make an array with images imgs and do imgs.length
- Instead of using a regex, add a data-current_img="1" to your images and use that to count
- If you use the data method, you can update the image to elem.src = imgs[current+1] to move to the next img, instead of the pricy regex

Now you don't have to manually limit to 3, or rename the images to their number, nor do you need to update our code if an extra state has to be build.

\$\endgroup\$
2
  • \$\begingroup\$ Thanks so much for kindly sharing your insight! I really appreciate it. \$\endgroup\$ – Rod Feb 10 '17 at 23:31
  • \$\begingroup\$ No problem. I've made a small change in round 2 in the regex, it should be more efficient now :) \$\endgroup\$ – Martijn Feb 13 '17 at 8:23

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.