Query, populate, show, hide data

Question

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

Answer 1

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);
});

• Quentin Pradet: collapse has two l and you still use .live() but I like it.
• InTry: 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
• natedavisolds: @Cygal ha! I thought that the spelling was off! I guess I should have looked it up.
• natedavisolds: @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.
• natedavisolds: @InTry Glad I could help. It is also good exercise for me. :) The edit only contains a few lines. Did it cut you off?

Answer 2

• .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 "-".

• InTry: $(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