1
\$\begingroup\$

Each row in the table has a delete and duplicate operation. The input values are passed to the new section based on the ids of each input on the row which changes dynamically based on the user's actions. The function for recreating the id's of the rows is not so good. I am not to sure how change it.

HTML

<table>
 <thead>
   <tr>
     <th>Package</th>
     <th>Weight</th>
     <th>Height</th>
     <th>Width</th>
     <th>Length</th>
     <th>Duplicate</th>
     <th>Delete</th>
   </tr>
 </thead>
 <tbody id="added-parcels">
   <tr id="1">
     <td><span>1</span></td>
     <td> 
        <div id="weighting-1" class="package-value">
           <input type="text" onkeypress="return isNumber(event)"> kgs 
         </div>
     </td>
     <td> 
        <div id="heighting-1" class="package-value">
            <input type="text" onkeypress="return isNumber(event)"> cm
        </div>
     </td>
     <td> 
        <div id="widthing-1" class="package-value">
            <input type="text" onkeypress="return isNumber(event)"> cm</div>
     </td>
     <td> 
        <div id="lengthing-1" class="package-value">
            <input type="text" onkeypress="return isNumber(event)"> cm</div>
     </td>
     <td>
        <button id="1" class="package-dup package-add-dup-1" title="Add Duplicate Parcel"></button>
     </td>
     <td><div class="package-delete package-button" title="Delete"></div>
     </td>
  </tr>

  <tr id="2">
     <td>
       <span>2</span>
     </td>
     <td> 
        <div id="weighting-2" class="package-value" onkeypress="return isNumber(event)">
           <input type="text"> Kg's
        </div>
      </td>
      <td>  
         <div id="heighting-2" class="package-value" onkeypress="return isNumber(event)"> 
             <input type="text"> cm
         </div>
       </td>
       <td> 
           <div id="widthing-2" class="package-value" onkeypress="return isNumber(event)">
              <input type="text"> cm
           </div>
        </td>
        <td> 
            <div id="lengthing-2" class="package-value" onkeypress="return isNumber(event)"> 
               <input type="text"> cm
            </div>
         </td>
         <td>
               <button id="2" class="package-dup package-add-dup-2" title="Add Duplicate Parcel"></button>
         </td>
         <td>
               <div class="package-delete package-button" title="Delete"></div>
          </td>
     </tr>
   </tbody>
 </table>

And the JS that handles the delete function that keeps the order of the values that are being dynamically generated:

//<!-- Recreate numbers on parcels
function reNumber() {
  $('#added-parcels tr td:first-child span').empty()

  $('#added-parcels tr').each(function () {
  $(this).attr('id',newNum++)
  });
  newNum = 1;
  $('#added-parcels tr td:first-child span').each(function () {
    $(this).text(newNum++);

  });
  newNum = 1;
  $('#added-parcels tr td:nth-child(2) div').each(function () {
     $(this).attr('id','weighting-' + newNum++);

  });
  newNum = 1;
  $('#added-parcels tr td:nth-child(3) div').each(function () {
    $(this).attr('id', 'heighting-' + newNum++);

  });
  newNum = 1;
  $('#added-parcels tr td:nth-child(4) div').each(function () {
    $(this).attr('id', 'widthing-' + newNum++);

  });
  newNum = 1;
  $('#added-parcels tr td:nth-child(5) div').each(function () {
    $(this).attr('id', 'lengthing-' + newNum++);

  });

  newNum = 1;
}

I don't think I can change the HTML. It is dependent on too many other operations to consider, but maybe just a more effective way to code the recreation of the ids when the reNumber() function is called.

\$\endgroup\$
1
  • \$\begingroup\$ Can you create a fiddle and also try to explain precisely what you are trying to do? Sorry, I couldn't understand your question very well. \$\endgroup\$ Commented Sep 19, 2013 at 12:22

1 Answer 1

Reset to default
1
\$\begingroup\$

Probably the most important thing you should be focusing on is the idea that ID's are unique. There can't be more than one element with the same ID. If you're trying to select several elements, jQuery will stop looking as soon as it hits the first element that mathces the ID. You should use classes instead. That said, it's up to you to make that change. I cannot determine how to do this, or the implications it will have on your site, just based off the code you posted.

Second most important thing is you should cache your jQuery selections. If you use a selection more than once, you should cache it. This way jQuery doesn't have to go looking for that element(s) every time. You then simply reference the variable you saved the selection in.

I've also replace your variable newNum inside the .each() callbacks to use i instead. This is the index. It counts how many times you've gone through this callback and is more appropriate for what you're trying to do. I would also recommend starting newNum at zero since JS is mostly zero-indexed and will make your life easier if you need to manipulate any of these ID's later.

Anyways, I'm sure there's more you could improve on, but without changing the HTML and while keeping this "the jQuery way" there's not much else I could change. Here are the few changes I've made:

function reNumber() {
    var parcelsTR = $("#added-parcels tr"),
        parcelsTRFirst = parcelsTR.find("td:first-child span"),
        newNum = 1;

    parcelsTR.each(function (i) {
        $(this).attr('id', newNum+i);
    });

    parcelsTRFirst.empty();

    parcelsTRFirst.each(function (i) {
        $(this).text(newNum+i);
    });

    parcelsTR.find('td:nth-child(2) div').each(function (i) {
        $(this).attr('id','weighting-' + newNum+i);
    });

    parcelsTR.find('td:nth-child(3) div').each(function (i) {
        $(this).attr('id', 'heighting-' + newNum+i);
    });

    parcelsTR.find('td:nth-child(4) div').each(function (i) {
        $(this).attr('id', 'widthing-' + newNum+i);
    });

    parcelsTR.find('td:nth-child(5) div').each(function (i) {
        $(this).attr('id', 'lengthing-' + newNum+i);
    });
}
\$\endgroup\$

You must log in to answer this question.