2
\$\begingroup\$

I am using keypress to get all the events of a input control. Once the user hits the enter button after I am then clone a div and update the ids. One project I am having is that I only want to do this one time for each control. Meaning if a user changes row 1 two times I dont want to do this logic. I have set this data field to 0 then change it to 1 but that is not working.

Any suggestions/comments are greatly appreciated to improve the code further. Thanks.

        $("[id^='fieldrowItem_']").on('keypress', function (e) {
            if (e.which === 13) {
                let rowDataChanged = $(this).data();
                //if (rowDataChanged === "0")
                //{

                    let rowCount = $('[id^=newrowItem_]').length
                    let newrowCount = rowCount + 1;

                    let rowField = $("[id='newrowItem_" + rowCount + "']").clone(true,true);
                    let rowInput = "input[id='fieldrowItem_" + rowCount + "']";                

                    let newrowInput = "fieldrowItem_" + newrowCount;

                    let placeholder = "Row " + newrowCount;

                    rowField = rowField.attr("id", "newrowItem_" + newrowCount + "");
                    rowField.find(rowInput)
                        .attr("id", newrowInput)
                        .attr("placeholder", placeholder)
                        .val('');

                    $("[id='newrowItem_" + rowCount + "']").after(rowField);

                    $(this).attr('data', "1");
               // }


            }
        });
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.0/jquery.min.js"></script>
<div class='form-group'>
  <div class="col-sm-6 row">
  <label>Rows</label>
  <div class="input-group" id=newrowItem_1>
  <input type='text' id='fieldrowItem_1' class='form-control fieldrowItem' placeholder="Row 1" data-value="0">
  <div class="input-group-addon">
  <span class="input-group-addon"  style="display: none;cursor:pointer;" data-section="admin" onclick="RemoveRow_1()">
  <i class="fa fa-times"></i>
  </span>
  </div>
  </div>                                            
</div>

What I am trying to replicate.

enter image description here

\$\endgroup\$
14
  • \$\begingroup\$ Have you considered using a <template> with the HTML you want to replace, but without settings any id (besides the one for the template)? \$\endgroup\$ Commented Jan 23, 2023 at 22:33
  • \$\begingroup\$ No I have not used a template element yet ? \$\endgroup\$
    – Jefferson
    Commented Jan 24, 2023 at 0:25
  • \$\begingroup\$ Instead of cloning a random element, you might want to take a look into it. It's extremely useful for situations like the one you have. Also, can you give a little more context, and convert your JSFiddle link into a StackSnipper (the icon on the right side of the image button)? \$\endgroup\$ Commented Jan 24, 2023 at 0:39
  • \$\begingroup\$ @IsmaelMiguel I have converted the JSFiddler into a StackSnipper and added some more context let me know if that helps \$\endgroup\$
    – Jefferson
    Commented Jan 24, 2023 at 12:58
  • \$\begingroup\$ Is this code working as expected? Because when I interact with it, it seems to do not do anything. \$\endgroup\$ Commented Jan 24, 2023 at 14:32

2 Answers 2

2
\$\begingroup\$

I'm not going to lie: the code is quite messy, and really hard to read.

The indentation is all over the place, and the way the JavaScript is written is extremely confusing.

I will split this review by language, to make it easier to follow.

However, first, I need to talk about ...



Hidden broken functionality

You have hidden code that isn't functional.

There's this block of HTML:

<div class="input-group-addon">
<span class="input-group-addon"  style="display: none;cursor:pointer;" data-section="admin" onclick="RemoveRow_1()">
<i class="fa fa-times"></i>
</span>
</div>

But, the code doesn't work or do anything at all.

Your way to make it just functional enough for a review was to hide the button.

A code review is supposed to review all the code, as such, I will review everything.

I'll talk about the JavaScript code, because it's a different beast...



CSS

In the previous block of HTML, you have inline CSS.
Please avoid it.

More often than not, you can just move those changes to a different class.

However, in this case, you don't even need that inline CSS at all, and can just discard it, since Bootstrap already has everything you need for this.

If you want to hide the element, just use the d-none class, and remove it if you need to show the element.

I won't go much further because you didn't provide the Bootstrap version and the font-awesome version.

For the cursor, I will talk about it in the HTML section.

Also, you do have extra classes that have absolutely no styles at all, like the class fieldrowItem.



HTML

The HTML has absolutely no indentation at all.
Besides making it really hard to read, also makes it really hard to follow what's what.
Also, the code is all glued together, making it look like a giant blob of text - very hard to read.

Not to mention the mix of quotes, between double-quotes, single-quotes and *gasp* no quotes :O.
Please, be consistent with the quotes (preferring the double-quotes), and ALWAYS have quotes.
If you don't use quotes, you can have lots of nasty surprises.

Here's how the HTML code could look like:

<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.0/jquery.min.js"></script>
<div class="form-group">
    <div class="col-sm-6 row">
        <label>Rows</label>
        
        <div class="input-group" id="newrowItem_1">
            <input type="text" id="fieldrowItem_1" class="form-control fieldrowItem" placeholder="Row 1" data-value="0">
            
            <div class="input-group-addon">
                <span class="input-group-addon" style="display: none;cursor:pointer;" data-section="admin" onclick="RemoveRow_1()">
                    <i class="fa fa-times"></i>
                </span>
            </div>
            
        </div>
    </div>

Now, we can dig deeper, and ... notice the missing closing </div>.

Count it in your code: you open 4 <div> but only have 3 </div>.


Regarding the hidden button, you have this code:

<div class="input-group-addon">
    <span class="input-group-addon" style="display: none;cursor:pointer;" data-section="admin" onclick="RemoveRow_1()">
        <i class="fa fa-times"></i>
    </span>
</div>

Instead of having this wrapped around a <span>, just use a button, like this:

<!-- Bootstrap 5.x -->
<div class="input-group">
    <input type="text" id="fieldrowItem_1" class="form-control fieldrowItem" placeholder="Row 1" data-value="0">
    
    <button class="btn btn-outline-secondary" type="button">
        <i class="fa fa-times"></i>
    </button>
</div>

<!-- Bootstrap 4.x -->
<div class="input-group">
    <input type="text" id="fieldrowItem_1" class="form-control fieldrowItem" placeholder="Row 1" data-value="0">
    
    <div class="input-group-append">
        <button class="btn btn-outline-secondary" type="button">
            <i class="fa fa-times"></i>
        </button>
    </div>
</div>

<!-- Bootstrap 3.x -->
<div class="input-group">
    <input type="text" id="fieldrowItem_1" class="form-control fieldrowItem" placeholder="Row 1" data-value="0">
    
    <div class="input-group-btn">
        <button class="btn btn-outline-secondary" type="button">
            <i class="fa fa-times"></i>
        </button>
    </div>
</div>

I don't know which version you're aiming for, so, here's for all versions.
I haven't tested this, but the functionality should be almost there.


And yes, I removed the old-school inline onclick event handler: we're in 2023, and that function doesn't even exist.



JavaScript and jQuery

This code is an absolute mess and very sub-optimal.
Did I mention that the indentation is a nightmare?

You're also using jQuery 3.3.0, instead of using jQuery 3.6.3 - the latest version.
There were some potentially serious vulnerabilities in jQuery 3.3.0, some of them being fixed in jQuery 3.5 and newer: https://www.cvedetails.com/product/11031/Jquery-Jquery.html?vendor_id=6538

One big mistake (that may or may not be slightly mitigated depending on where the code is included) is to do not run this code on a ready event.

Just simply wrapping your code in something like this:

jQuery(function($){
    // Your code comes here...
});

This will solve any possible issue.

If you hate using braces, you can have the JavaScript at the end of the <body> element, but this code does more than you see...
In case someone/something runs jQuery.noConflict(), your code is perfectly safe and can continue using the $ "shortcut" for jQuery.


You have absolutely useless comments, like this one: //if (rowDataChanged === "0").
If you want to remove the code, please just remove it, instead of having it dangling there doing nothing.

And your code is in need of comments: the duality of coding.
Your code is void of any explanation as to why you decide to do what you're doing.

Important stuff like:

  • Why do you have the ids based in a counter, based on the length?
  • Why are you cloning a <div> instead of using a template?
  • What's that $(this).attr('data', "1"); doing there?
  • Why did you decide to check for keypresses, instead of using the input event?

This is the type of stuff that would really help in being added to the code.




How I would implement this?

Yeah, I've been "bashing" your code for a while, instead of giving an example of how I would implement this.

I've implemented some basic functionality for the delete button - as you deserve it for paying attention.

I've changed the functionality on how you add the rows, to make it more obvious to the user, as pressing "enter" isn't something that obvious.
In fact, can you even press "enter" on a mobile device, on a single-line input?
If I'm not mistaken, it is just replaced by an emoji button, for those fields...

This isn't very accessible for assistive technologies, and I didn't make an effort for that (I leave it as an exercise for you, since I'm too lazy).

Despite being A LOT longer than your code, I'm sure it is infinitely easier to read and understand what's going on.

jQuery(function($){
  var $container = $('#field-container');
  // To work with <template>, you can't use jQuery's API
  var template = $('#template-input-group').get(0);
  
  // Easy access to selectors that are repeated - in case we need to change them
  const INPUT_SEL = 'input[type="text"][name="name[]"]';
  const ROW_SEL = '.row';
  const DELETE_SEL = '.btn.delete';
  
  
  // Creates the first row, since $container starts empty
  addRow();
  
  
  function addRow(){
    // Must use .cloneNode(true) for a deep clone
    var $input_group = $(template.content.cloneNode(true));
    
    $container.append($input_group);
  }
  
  
  // My best interpretation of what the hidden remove button is supposed to do
  function removeRow($row){
    // In case something goes wrong, don't remove the last row
    if($row.is(':last-child'))
    {
      // Set the input to have an empty value - removes undo/redo as well
      $row.find(INPUT_SEL).val('');
      return;
    }
    
    // Can't move this to the top because the code needs to check if it is the last element before removing
    $row.remove();
  }
  
  
  // Handles changes in the input, and add a row if appropriate
  $container.on('input', INPUT_SEL, function(){
    var $this = $(this);
    var $row = $this.closest(ROW_SEL);
    var $btn_delete = $row.find(DELETE_SEL);
    
    // Enable the button, just in case it is still disabled
    $btn_delete.removeAttr('disabled');
    
    /*
      Only add a new row if the user is in the last row.
      Also check if the value is empty, just to prevent bugs.
      This check isn't really necessary, but prevents weird edge cases I couldn't predict.
    */
    if($row.is(':last-child') && $this.val() !== '')
    {
      addRow();
    }
  });
  
  // Handles the delete button - to remove a row
  $container.on('click', DELETE_SEL, function(){
    var $this = $(this);
    var $row = $this.closest(ROW_SEL);
    
    removeRow($row);
  });
});
/* Adds a counter to the rows - matches the example image */
#field-container {
  /* Guarantees that the counter exists */
  counter-reset: rows;
}

#field-container .input-group::before {
  /* Shows the counter - no JS required */
  counter-increment: rows;
  content: counter(rows) ".";
  
  align-self: center;
  
  /*
    The baseline width is 3 characters.
    This should be fine up to 99 rows.
  */
  width: 3ch;
}


#field-container input {
  /* Remove the border radius, to make it look closer to the example image */
  border-radius: 0;
}

#field-container .btn.delete {
  /* Ridiculous padding was removed */
  padding: 0 0.375rem;
  
  /* Make sure the delete button is always gray */
  --bs-btn-color: var(--bs-gray);
  --bs-btn-hover-color: var(--bs-gray);
  --bs-btn-active-color: var(--bs-gray);
}

#field-container .btn.delete:disabled {
  /* Make it more obvious that the button is disabled */
  opacity: 0.5;
}

#field-container .btn.delete > svg {
  /* The SVG icons should always follow the .btn font-size */
  width: 2.5em;
  height: 2.5em;
}
<!-- Hide the SVG, since it doesn't need to be visible -->
<svg xmlns="http://www.w3.org/2000/svg" class="d-none" fill="currentColor">
  <!-- Modified version of: https://icons.getbootstrap.com/icons/x/ -->
  <symbol id="icon-close" fill="currentColor" viewBox="0 0 16 16">
    <path d="M4.646 4.646a.5.5 0 0 1 .708 0L8 7.293l2.646-2.647a.5.5 0 0 1 .708.708L8.707 8l2.647 2.646a.5.5 0 0 1-.708.708L8 8.707l-2.646 2.647a.5.5 0 0 1-.708-.708L7.293 8 4.646 5.354a.5.5 0 0 1 0-.708z"/>
  </symbol>
</svg>


<!-- The template with the basic HTML to use -->
<template id="template-input-group">
  <!-- mb-3 = margin bottom -->
  <div class="row mb-3">
    <div class="input-group">
    
      <!-- To be useful, server-side, must have a name -->
      <input type="text" name="name[]" class="form-control" placeholder="Row name" />

      <!-- Must be disabled by default, to prevent deleting the last row by mistake -->
      <button class="btn btn-link delete" type="button" disabled>
        <!-- Icon from the SVG on top -->
        <svg role="img" aria-label="Remove">
          <use href="#icon-close"></use>
        </svg>
      </button>
      
    </div>
  </div>
</template>


<!-- Stops the user from selecting everything if the user double-clicks the disabled button -->
<div class="container user-select-none">
  <div class="form-group col-6">
  
    <p>Rows</p>
    
    <div id="field-container"></div>
    
  </div>
</div>

<!-- Use CDNJS instead of code.jquery.com because I'm lazy and it comes with all the hashes and stuff -->
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.6.3/jquery.min.js" integrity="sha512-STof4xm1wgkfm7heWqFJVn58Hm3EtS31XFaagaa8VMReCXAkQnJZ+jEy8PCC/iT18dFy95WcExNHFTqLyp72eQ==" crossorigin="anonymous" referrerpolicy="no-referrer"></script>

<!-- Using the latest stable version -->
<link href="https://cdn.jsdelivr.net/npm/[email protected]/dist/css/bootstrap.min.css" rel="stylesheet" integrity="sha384-rbsA2VBKQhggwzxH7pPCaAqO46MgnOM80zW1RWuH61DGLwZJEdK2Kadq2F9CUG65" crossorigin="anonymous">

This code relies in the jQuery.closest function, to find the parent .row element.
This makes it possible to find the row without having to fiddle around with ids and stuff like that.

The name on the inputs looks strange, if it is your first time seeing it (E.g.: name[]).
The idea is, in case you submit it (or serialize it), you get an array with all the values.
On the server-side, you will have an array with all the values.

The disadvantage on how I have this implemented is that the last item is always empty.
You should filter these out server-side anyway, as you can have any field being empty anywhere...


If you have any questions, just send a comment and I will explain everything, and even add an explanation to this answer.

\$\endgroup\$
11
  • \$\begingroup\$ Thanks for the review one question is how would I get the collections of ‘name[]‘ ? \$\endgroup\$
    – Jefferson
    Commented Jan 28, 2023 at 23:58
  • \$\begingroup\$ @Jefferson That depends on what language you're talking about, and if it is in the client or server-side. \$\endgroup\$ Commented Jan 28, 2023 at 23:59
  • \$\begingroup\$ So I have to build a object client side with all the data and then make a Ajax call . \$\endgroup\$
    – Jefferson
    Commented Jan 29, 2023 at 0:01
  • \$\begingroup\$ @Jefferson Then you should have a <form> somewhere. On that form, you add an event listener to the <form> and then just do $(this).serialize() into $.post() or $.ajax(). api.jquery.com/serialize <-- This explains everything. Since none of that code is part of the review, this is as far as I can help. If you check the $.post documentation (api.jquery.com/jQuery.post/…) you can see that the data argument accepts a PlainObject like {field: 'data'} or String (which is the result of $.serialize). \$\endgroup\$ Commented Jan 29, 2023 at 0:03
  • 1
    \$\begingroup\$ @Jefferson I've added the requested counter. 100% CSS, which means, you have to do a lot less work and you don't have to implement anything in JavaScript. As long as it has it's little space, it will always be there. \$\endgroup\$ Commented Jan 29, 2023 at 0:54
1
\$\begingroup\$

Simply remove the handler once it's invoked if you don't want the add-row behaviour to happen twice.

        $("[id^='fieldrowItem_']").on('keypress', function keyPressHanlder (e) {
            if (e.which === 13) {
                let rowDataChanged = $(this).data();

                    let rowCount = $('[id^=newrowItem_]').length
                    let newrowCount = rowCount + 1;

                    let rowField = $("[id='newrowItem_" + rowCount + "']").clone(true,true);
                    let rowInput = "input[id='fieldrowItem_" + rowCount + "']";                

                    let newrowInput = "fieldrowItem_" + newrowCount;

                    let placeholder = "Row " + newrowCount;

                    rowField = rowField.attr("id", "newrowItem_" + newrowCount + "");
                    rowField.find(rowInput)
                        .attr("id", newrowInput)
                        .attr("placeholder", placeholder)
                        .val('');

                    $("[id='newrowItem_" + rowCount + "']").after(rowField);

                    $(this).attr('data', "1");

                    $(this).off('keypress', keyPressHanlder);
            }
        });
<script src="https://cdnjs.cloudflare.com/ajax/libs/jquery/3.3.0/jquery.min.js"></script>
<div class='form-group'>
  <div class="col-sm-6 row">
  <label>Rows</label>
  <div class="input-group" id=newrowItem_1>
  <input type='text' id='fieldrowItem_1' class='form-control fieldrowItem' placeholder="Row 1" data-value="0">
  <div class="input-group-addon">
  <span class="input-group-addon"  style="display: none;cursor:pointer;" data-section="admin" onclick="RemoveRow_1()">
  <i class="fa fa-times"></i>
  </span>
  </div>
  </div>                                            
</div>

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Also if you want to know why your 0 & 1 trick didn't work - two problems 1) your data-value is integer, not string, you can verify that by console.log 2) $(this).attr('data', "1"); is setting data="1", but you need $(this).data("value", 1) instead \$\endgroup\$ Commented Jan 25, 2023 at 12:39

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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