2
\$\begingroup\$

I have created code to show/hide data from a DB. It works, but as I am new to jQuery, I would like to know if this is a good way.

HTML:

<li class="devInfo">
    <span class="sn_table"><? echo $sn; ?></span>       
    <span class="last_edit_table"><? echo $last_change; ?></span>        
    <span class="comment_table"><? echo $comment; ?></span>
    <span class="model_table"><? echo $model_name; ?></span>
    <a class="more" data-name="<? echo $id; ?>" href="">+</a>
    <div class="details"></div>
</li>                       

jQuery:

$(function () {
    $(".more").live("click", function () {            
    var onMe =$(this);
    var detailsConteiner = $(onMe).next('div');            
    var forLoader = $(onMe).parent("li");

        if ($(onMe).text() == "+") {

            $(devInfo).children("span").css({"text-indent":"", "text-overflow":""});
            $(forLoader).fadeIn(400).prepend('<img  src="/images/css/loader_small.gif" style="margin-bottom:.25em; margin-left:1.25em; vertical-align:middle;">');
            $(onMe).text("-")
            $(devInfo).css("background", "");
            $(forLoader).css("background", "#d6e5f4");
            $(onMe).siblings("span").css({"text-indent":"1000px", "text-overflow":"clip"});            


            $(detailsConteiner).empty();
            $(details).slideUp(1000);

            var value = $(onMe).attr("data-name");
            var dataString = 'deviceId=' + value;

            $.ajax({
                type: "POST",
                url: "helpers/getDetails.php",
                data: dataString,
                cache: false,

                    success: function (html) {    


                        $(detailsConteiner).prepend(html).slideDown(500);    
                        $(forLoader).children("img").fadeOut(600);                

                    }                
            });
            return false;
        }            
        else {

            $(details).slideUp(700);
            $(onMe).text("+");
            $(forLoader).css("background", "");
            $(onMe).siblings("span").css({"text-indent":"", "text-overflow":""});
            return false;
        }
    });
});

Ajax response:

<dl class="details">
    <dd>Id: <? echo $id; ?> </dd>
    <dd>Serial: <? echo $sn; ?> </dd>    
    <dd>Model: <? echo $model_name; ?> </dd>
    <dd contenteditable  data-name="comment">Comment:<? echo $comment; ?></dd>
    <dd>Last Change<? echo date('d.m.Y', strtotime($last_change)); ?> </dd>
</dl>

CSS:

.devInfo  img {
    margin-bottom:.25em; 
    margin-left:1.25em; 
    vertical-align:middle;
margin-left:25px;
 }

li.expanded {
    background-color: #d6e5f4;
}


li.expanded span {
    text-indent: -1000px;
    text-overflow:clip;
}

JS:

*function showDetails() // is not needed as the function insertDetails is doing job

function hideRestOfLI() {            
         $(".details").slideUp(1000);
    }

function expand() {
        hideRestOfLI(); 
        $(".devInfo").removeClass(ExpandClass);
        $(".more").text(ColapsedChar);
        $devInfo.addClass(ExpandClass);
        $moreLink.text(ExpandedChar);           

    }




 completed: hideLoading // complete: hideLoading
\$\endgroup\$
2
\$\begingroup\$

There are many changes to the css through jquery that could just be handled by adding a class to devInfo.

I...

  1. Changed some variable names for readability
  2. Created some variable constants for readability
  3. Created single responsibility functions for clean code
  4. Only transformed elements to jQuery once and stored in a variable starting with '$'
  5. Extracted out the style from js and put it in css
  6. Prevented the link's default action from going
  7. Added functionality to add/remove the 'expanded' class

The code is completely untested. So there is probably some syntax issues, but let me know what you think.

CSS

.devInfo a.more img {
    margin-bottom:.25em; 
    margin-left:1.25em; 
    vertical-align:middle;
}

.devInfo.expanded a.more {
    background-color: #d6e5f4;
}

.devInfo.expanded {
    background: url();
}

.devInfo.expanded .span {
    text-indent: 1000px;
    text-overflow: clip;
}

JS

$(function () {
    var ExpandedChar = "-",
        ColapsedChar = "+",
        ExpandClass = 'expanded';

    var toggleDetails = function(event) {   
        event.preventDefault();

        var $moreLink = $(this),
            $details= $moreLink.next('.details'), 
            $devInfo = $moreLink.closest('.devInfo');

        var displayLoading = function() {
            $devInfo.prepend(buildLoadImage()).fadeIn(400);
        };

        var hideLoading = function() {
            $devInfo.children("img").fadeOut(600);
        };

        var insertDetails = function(html) {  
            $details.prepend(html).slideDown(500);                
        };

        var getDetailsFor = function(deviceId) {
            $.ajax({
                 type: "POST",
                 url: "helpers/getDetails.php",
                 data: {deviceId: deviceId},
                 cache: false,
                 beforeSend: displayLoading,
                 success: insertDetails,
                 completed: hideLoading
             });
        }

        if (isColapsed()) {
            expand();
            getDetailsFor($moreLink.attr("data-name"));      
        } else {
            colapse();
        }    

        function expand() {
            $devInfo.addClass(ExpandClass);
            $moreLink.text(ExpandedChar);
            showDetails();
        }

        function showDetails() {
            $details.slideUp(1000);
        }

        function colapse() {
            $devInfo.removeClass(ExpandClass);
            $moreLink.text(ColapsedChar);
            hideDetails();
        }

        function hideDetails() {
            $details.slideUp(700);
        }

        function isColapsed() {
            return $moreLink.text() == ColapsedChar;
        }

        function buildLoadImage() {
            return $('<img />').attr('src', '/images/css/loader_small.gif');
        }
    }

    $(".more").live("click", toggleDetails);
});
\$\endgroup\$
  • \$\begingroup\$ collapse has two l and you still use .live() but I like it. \$\endgroup\$ – Quentin Pradet Mar 8 '12 at 5:50
  • \$\begingroup\$ Thank you very much for your code review, you show me complitly different way how to structure code, I learned a lot. Its took a lot of time to understand/debug the code but with a few changes i make it works, please see the EDIT \$\endgroup\$ – InTry Mar 8 '12 at 20:01
  • \$\begingroup\$ @Cygal ha! I thought that the spelling was off! I guess I should have looked it up. \$\endgroup\$ – natedavisolds Mar 9 '12 at 0:10
  • \$\begingroup\$ @Cygal I thought of using delegate or on (and use prop or data instead of attr) but I didn't know what version of jQuery was being used. So I decided to stick with what I saw. \$\endgroup\$ – natedavisolds Mar 9 '12 at 0:13
  • \$\begingroup\$ @InTry Glad I could help. It is also good exercise for me. :) The edit only contains a few lines. Did it cut you off? \$\endgroup\$ – natedavisolds Mar 9 '12 at 0:15
1
\$\begingroup\$
  • .live(): As of jQuery 1.7, the .live() method is deprecated for various reasons. Users should go for a combination of .on() and .delegate(). For this code, you don't need to use .delegate() since a.more is never dynamically modified. Simply use .on() like this:

    $("a.more").on("click", function() { ... });
    

    (If you were attaching an event on a lot of elements, you would have used another construct, see the .on() doc for details.)

  • Dead code:

    $(devInfo).children("span").css({"text-indent":"", "text-overflow":""});
    

    The first time you write this, it seems to have no effect. Remove it if this is the case.

  • Timeouts: Whenever you're doing Ajax calls, don't forget that requests could timeout, especially on phones. $.ajax() provides you with a way to handle timeouts, do it, and revert a.more to "+".
  • Use strict equality comparison.
  • You could use a switch on $(onMe).text(), with two cases: "+" and "-".
\$\endgroup\$
  • \$\begingroup\$ $(devInfo).children("span").css({"text-indent":"", "text-overflow":""}); // it's not dead, its reset css after it has been clicked on the one of the a.more \$\endgroup\$ – InTry Mar 7 '12 at 21:38

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.