0

I'm just facing a problem while clicking on "Add New Item" button, javascript won't add me a new line (<li>) in <ul>

I tried making a variable for <ul> with id="change" which is document.getElementById("change");

On the other hand, I just added an "addEventListener" to this variable, so while I click on this button, a function named "achange" will start working, and add a new line to the <ul>

var icounter = 1;
var c = document.getElementById("change");
c.addEventListener("click", achange);

function achange() {
  ulist.innerHTML += "<li> New Item " + icounter + "</li>";
}
<h1 id="header"> List that contains items. </h1>
<button id="change">Add New Item </button>
<ul type="square" id="ulist">
  <li> Pizza </li>
  <hr align="left" width="20%">
  <li> Burger </li>
  <hr align="left" width="20%">
  <li> Chicken Dinner </li>
  <hr align="left" width="20%">
  <li> Salad </li>
  <hr align="left" width="20%">
</ul>

I'm expecting to add a line which says : "New Item 0" , but nothing happens, any help would be appreciated. Thank you

Tyler Roper
  • 21,445
  • 6
  • 33
  • 56
  • use document.getElementById('ulist').innerHTML += .... – sjdm Jan 17 '19 at 22:44
  • Your code seems to work fine. Have you put your `addEventListener` in the ``, *without* wrapping it in a `Window.load` (or equivalent)? Also, `
    ` **is not a valid child of `
      `.** Your HTML is invalid.
    – Tyler Roper Jan 17 '19 at 22:46
  • 1
    @TylerRoper Not all browsers support id access by properties on the window. OP's browser probably doesn't support that. – Spencer Wieczorek Jan 17 '19 at 22:47
  • @sjdm Take another look at OP's code. He's already doing `innerHTML += ...`. – Tyler Roper Jan 17 '19 at 22:47
  • @SpencerWieczorek That makes more sense. – Tyler Roper Jan 17 '19 at 22:47
  • I didn't even realise you could do that. It seems wrong somehow. – Andy Jan 17 '19 at 22:48
  • Your HTML is **invalid**. `hr` cannot be a direct child element of `ul`, only `li` is allowed. – connexo Jan 17 '19 at 22:49
  • Funny enough, despite its lack of support across all browsers, named access on the `window` object is actually specifically *included* in the [HTML5 spec]( https://html.spec.whatwg.org/#named-access-on-the-window-object), though it clarifies: *"As a general rule, relying on this will lead to brittle code. Which IDs end up mapping to this API can vary over time, as new features are added to the Web platform, for example. Instead of this, use `document.getElementById()`"* – Tyler Roper Jan 17 '19 at 22:50
  • @connexo so removing hr may fix my code? – Raphael Eid Jan 17 '19 at 22:50
  • No, but invalid HTML is inacceptable nonetheless. – connexo Jan 17 '19 at 22:51
  • @connexo i just removed hr and nothing happened – Raphael Eid Jan 17 '19 at 22:51
  • @connexo so what should i do to fix this button? – Raphael Eid Jan 17 '19 at 22:52
  • @RaphaelEid Change `ulist.innerHTML` to `document.getElementById("ulist").innerHTML` and see if that resolves your issue. If not, then it's likely because your click event handler is not waiting for the page to load. – Tyler Roper Jan 17 '19 at 22:52
  • @TylerRoper i tried it sir, nothing happens, so the problem is from webpage/browser or my code? – Raphael Eid Jan 17 '19 at 22:53
  • The JavaScript you've provided - is it in the `` of your document? And, if so, is it wrapped in another event? – Tyler Roper Jan 17 '19 at 22:54
  • @jonathana Using inline event listeners is widely considered bad practice. – connexo Jan 17 '19 at 23:31

4 Answers4

2

You're getting things at the wrong time. I would also recommend a different way of appending your items rather than append text into innerHTML.

Within your function (when you add the item) you'll want to get the reference to your ul and append children from there. This also gives you the opportunity to get the count of lis for numbering the next item (if you wish)

Also, as pointed out by others, not all browsers will support the id reference you're trying to do with ulist

Another thing to mention here is that hr is not a valid decendent of ul. If you really want the hr, you should use css or put the hr within the li (Is anything except LI's allowed in a UL?)

const addNewItem = () => {
  const list = document.getElementById('ulist');
  const liCount = list.querySelectorAll('li').length;
  const hr = document.createElement('hr');
  hr.setAttribute('align', 'left');
  hr.setAttribute('width', '20%');
  const newLi = document.createElement('li');
  newLi.innerText = `Item # ${liCount + 1}`;
  newLi.append(hr);
  list.appendChild(newLi);
};

document.getElementById('change').addEventListener('click', addNewItem);
<h1 id="header"> List that contains items. </h1>
<button id="change">Add New Item </button>
<ul type="square" id="ulist">
<li> Pizza </li> <hr align="left" width="20%">
<li> Burger </li> <hr align="left" width="20%"> 
<li> Chicken Dinner </li> <hr align="left" width="20%">
<li> Salad </li> <hr align="left" width="20%">
</ul>
mwilson
  • 12,295
  • 7
  • 55
  • 95
  • 2
    This is a good answer, and I know that OP's example includes the `
    `s, but it's worth pointing out that they're completely invalid. The only acceptable direct child of a `
      ` element is an `
    • ` element.
    – Tyler Roper Jan 17 '19 at 22:53
  • Good point. I personally would have used css for that part, but added it in anyways. – mwilson Jan 17 '19 at 23:00
  • @mwilson i used #change{ background-color:black; border-radius:50%; padding-left:50px; height:20%; font-family:Arial; font-weight:bold; color:red; padding-right:37px; } – Raphael Eid Jan 17 '19 at 23:04
  • What's invalid about it? – mwilson Jan 17 '19 at 23:07
  • `list.appendChild(hr)` appends a disallowed element to `list`. The fact that OPs initial HTML is already invalid does not justify suggesting Javascript that creates equally invalid results. – connexo Jan 17 '19 at 23:08
  • I added it in there for @Tyler Roper's feedback. Read the comments. – mwilson Jan 17 '19 at 23:12
  • 1
    You misinterpreted @TylerRoper's first comment. He criticizes the same as I do. – connexo Jan 17 '19 at 23:13
  • @connexo Is correct. I was suggesting that you point OP in the right direction by removing any invalid HTML. – Tyler Roper Jan 17 '19 at 23:14
  • Let alone the fact that `align` and `width` are deprecated attributes, and have been so since the end of WW II. – connexo Jan 17 '19 at 23:16
  • 1
    Tough crowd today. The answer is updated to address all concerns – mwilson Jan 17 '19 at 23:18
  • 1
    Eh. I just think if someone asks you to change their oil and you notice they're missing a tire, the least you could do is say somethin'. In any case, the answer is better now for mentioning it. – Tyler Roper Jan 17 '19 at 23:19
  • The answer is still creating invalid HTML. Fixing that is not optional, as your current edit suggests "*you should*". – connexo Jan 17 '19 at 23:30
  • Feel free to correct it or provide another answer if you're that concerned about it. – mwilson Jan 17 '19 at 23:34
  • You are responsible for your answers, not others. – connexo Jan 17 '19 at 23:36
0

4 issues with your code:

  1. Your HTML was invalid as I commented on your question. I removed the hr elements.
  2. Instead of working with strings and innerHTML, on simple elements it is often easier (and almost always much faster) to use document.createElement and appendChild methods.
  3. Your button had no type attribute, so it was type="submit" (which is the default type).
  4. You need to make sure the Javascript executes only after the DOM has been parsed, otherwise change and ulist are undefined. There is several ways to achieve that:

    • Wrap the functionality in a function that gets executed on DOMContentLoaded event (see below code).
    • Make sure the Javascript is positioned at the end of the body, right before the closing tag </body>.
    • Put your Javascript in an external file script.js and reference that in your HTML using <script src="path/to/script.js" defer></script>.

var icounter = 1;

document.addEventListener('DOMContentLoaded', () => {
  change.addEventListener("click", achange);

  function achange() {
    const li = document.createElement('li');
    li.textContent = `New Item ${icounter++}`;
    ulist.appendChild(li);
  }
})
<h1 id="header"> List that contains items. </h1>
<button id="change" type="button">Add New Item </button>
<ul type="square" id="ulist">
  <li> Pizza </li>
  <li> Burger </li>
  <li> Chicken Dinner </li>
  <li> Salad </li>
</ul>
connexo
  • 53,704
  • 14
  • 91
  • 128
  • Thank you sir for your time, is there any way to run it without using "let" and "textContent" and "appendChild" ? or this is the only way? – Raphael Eid Jan 17 '19 at 23:02
  • `li` should be `const`. The value of `li` is never changed. – mwilson Jan 17 '19 at 23:03
  • @mwilson Thanks for pointing out. Even though this is kind of personal preference, I agree and have changed it. – connexo Jan 17 '19 at 23:06
  • @RaphaelEid You can also use `innerHTML` the way you tried instead. I mentioned the cons for that in the answer. – connexo Jan 17 '19 at 23:40
0

You can also do this without using appendChild:

var iCounter = 1;

var c = document.getElementById("change");
c.addEventListener("click", achange);

function achange() {

  var ulist = document.getElementById('ulist');
  
  ulist.innerHTML += '<li> New Item ' + (iCounter++) + ' </li><hr align="left" width="20%">';
  
}
<h1 id="header"> List that contains items. </h1>
<button id="change">Add New Item </button>
<ul type="square" id="ulist">
  <li> Pizza </li>
  <hr align="left" width="20%">
  <li> Burger </li>
  <hr align="left" width="20%">
  <li> Chicken Dinner </li>
  <hr align="left" width="20%">
  <li> Salad </li>
  <hr align="left" width="20%">
</ul>
Monarth Sarvaiya
  • 1,041
  • 8
  • 20
0

I found a solution without appendChild and const and $ :

I just added onclick="functionName()" on the button and it worked.

<h1 id="header"> List that contains items. </h1>
<button id="change" onclick="achange()">Add New Item </button>
<ul type="square" id="ulist">
<li> Pizza </li> <hr>
<li> Burger </li> <hr>
<li> Chicken Dinner </li> <hr>
<li> Salad </li> <hr>
</ul>

 

var icounter = 1;
    function achange(){
    document.getElementById("ulist").innerHTML += "<li> New Item " +icounter +"</li> <hr>";
    icounter++;
    }