[JavaScript] Assistance Greatly Appreciated

I really don’t have any “how to” advice to give, but I would like to mention a couple of things:

First, I find the onclick attribute to be outdated and obtrusive. It really does not need to be in the HTML. There shouldn’t be any JS in the HTML, in my view. That is why event listeners were invented. Give the elements an id or class as applies and register the listeners in the JS.

Second, is it really necessary to have a prepared table with empty elements? Why not template it and only draw the rows that display registered members. If there is to be a limit on the size of the roster, then when that limit is reached, disable the Add Member button, or hide it.

Whatever you do, let it be delegation and not individual bindings to the event listeners. For instance, rather than individual bindings on each of the four '<button>s, bind to the parent object. A click detected anywhere in its domain will trigger the handler. At that point the delegation begins… Which element was clicked? Given that we know which handler to flow through.

The beauty of delegation is that for one it has way less bindings, meaning less memory is used to manage them, and there is no restriction on the size of the domain. We can dynamically add and remove buttons without paying any mind to event handling. It’s all taken care of.

Take for instance the Add Member button. It may be removed and reinserted without any care for what event handler it may trigger. It triggers the parent, who knows best what purpose the button serves, which picks up on the event.

3 Likes

I wouldn’t suggest checking if a variable is defined in this way. (a !== undefined) - it could trigger the error “a is not defined”. It is better to use typeof operator - you can’t go wrong with it :slight_smile: ex. (typeof a !== "undefined"). Remember the double quotes though, typeof operator returns a string.

3 Likes

document.getElementById().innerHTML works as intended of course. I used console in your jsbin, document.getElementById("memberName1").innerHTML = "TEST" and it worked. Problem definately lies elsewhere. I’d suggest not using onclick attribute on html elements - even this jsbin doesn’t seem to let them work. Add an event listener to the button in your javascript. Here you have some example:

   const myButton = document.getElementById("myButton");
   myButton.addEventListener("click", sayHello)
  
   function sayHello() {
     console.log("Hello!");
   }
2 Likes

The above example is exactly how not to set the event bindings. Use delegation as suggested earlier…

See also: In Depth - Event Delegation in JavaScript


Here is something to consider so you can assign a new id to every new class member.

let memberId = 0
class Member {
    static setId() {
      return ++memberId;
    }
    constructor(name) {
      this.name = name;
      this.memberId = Member.setId();
    }
}

const bill = new Member("Bill Bourne");
const bob = new Member("Bob Jackson");
Native Browser JavaScript
 >
=> undefined
 > bill.memberId
=> 1
 > bob.memberId
=> 2   
3 Likes

How does one accomplish this?

I’ve made certain changes (you can find those here), but am unsure how to properly accomplish what you are suggesting…

1 Like

Given the following HTML,

<!DOCTYPE html>
<html>
  <head>
    <meta charset="utf-8">
    <meta name="viewport" content="width=device-width">
    <title>delegate.it</title>
    <link href="index.css" rel="stylesheet" type="text/css" />
  </head>
  <body>
    <div id="buttons">
      <button>Create Roster</button>
      <button>Add Member</button>
      <button>Add Member + Check Them In</button>
      <button>Clear Roster</button>
    </div>
    <script src="index.js"></script>
  </body>
</html>

we will note that there is no JS in the markup. The following script will register an event listener on the parent element, and the suggested code from the Sitepoint article will guide us the rest of the way…

const buttons = document.querySelector('#buttons');

const newRoster = () => alert('New Roster');
const runInstance = flag => alert(flag);
const clearRoster = () => alert('Clear Roster');

//https://www.sitepoint.com/javascript-event-delegation-is-easier-than-you-think/
const getEventTarget = e => {
  e = e || window.event;
  return e.target || e.srcElement;
};

const buttonHandler = e => {
  let target = getEventTarget(e);
  switch (target) {
  case buttons.children[0]: newRoster(); break;
  case buttons.children[1]: runInstance(false); break;
  case buttons.children[2]: runInstance(true); break;
  case buttons.children[3]: clearRoster();
  }
};

buttons.addEventListener('click', buttonHandler);

Event Delegation example - Replit

Note the order that the code is written in. We define our DOM objects first, followed by the callback list, then the handlers, and lastly the event listener is attached. It matters because the functions have to exist already when they are called from the handlers.

3 Likes

I agree with you on event delegation of course. When there’s more events to listen for, then of course it’s pointless to have tens or hundreds of lines doing the same thing, but for different elements. My example was to show how to set event listener outside of onclick attribute in html.

2 Likes

Hey @mtf,

I’ve done quite a bit more since we last spoke. I’m getting there with it, but I’m not sure that I’m doing this properly…

Here is what I have so far. Check it out, see how I’m doing. Let me know if there anything I need to fix.

If everything is looking good so far, let me know what my next step should be, as I am lost on that front…

Do I need to make an event handler for each type of button? Does each individual member need their own event handler? How do you use an event handler to update the text within a cell?

1 Like

I have given quite a lot of input, already. Work up a couple of small side projects (similar to the demo I wrote for you above) and dig up the reading materials on events. Spend a week on the subject and dig for examples.

All the buttons in the cells (rows) need is one handler. Again, attach the listener to the parent object, and delegate. If you have two buttons on the row, then your handler will have two cases and respective callbacks.

const rowButtons = querySelector('#roster');

The id would go on the table element, being it is the parent of all the table rows. Clicks in any of them will bubble up to the parent, which can then determine which button was clicked, check in or check out, and assign the handler.

The event has an eventObject with a this property which is the context of the click event. (The row the button is in can be determined by traversing upwards to the nearest TR.) Each row has sibling TD’s and the one your event callbacks will be modifying will be the fourth, or last sibling in the group. Use a query selector to target that element then swap out the text node for that cell.

I still don’t feel comfortable having a static table with hardwired id attributes on the rows. When a member is added would be the time to add a row to the table, from a template (a script that composes the HTML and inserts the dynamic data, then appends it to the TBODY as a new TR.

This is not an exact version, but suggestive of what is needed in your template.

`        <tr id="${this.memberId}">
          <td><div class="name">${this.memberName}</div></td>
          <td><div><button class="checkIn">Check In</button></div></td>
          <td><div><button class="checkOut">Check Out</button></div></td>
          <td><div class="bool">${this.isCheckedIn}</div></td>
        </tr>`

These will be added to the handler cases to call (though not in their present form)

onclick="poolRoster[0].checkIn()"
onclick="poolRoster[0].checkOut()"

I’ll leave you to it to do some more research and experimentation that will help you get more familiar with events and dynamic construction of your page.

3 Likes

Shouldn’t the roster label be given to tbody instead of table? That way it ignores the th elements?

1 Like

It mattters more that you are able to add interaction anywhere in the table, and have all of it bubble up to the table parent. The entire table is a hot zone, not just the buttons. One parent delegating all actions makes the most sense to me.

3 Likes

Gotcha’. So I should give table an id of roster and specify within the handler that I only want to deal with the td elements.

1 Like

The TD elements will not be the targets. The buttons will. However, if you wish to make the TD’s interactive, then a case would be needed for those event targets. The target should be specific so the correct callback is fired by the handler.

For an exercise, use the template snippet above and write the code that will traverse from the event target (a button) up to the element with the id attribute, and read off the value. Use traversal methods.

3 Likes

Are you talking about this?

Also, I am currently receiving the following error…

On jsbin…

"error"
"TypeError: Cannot set property 'innerHTML' of null
    at Member.memberRow (moheciw.js:89:52)
    at addMember (moheciw.js:133:12)
    at runInstance (moheciw.js:199:9)
    at HTMLDivElement.buttonHandler (moheciw.js:230:13)"

On repl…

TypeError: Cannot set property 'innerHTML' of null
    at Member.memberRow (index.js:89:52)
    at addMember (index.js:133:12)
    at runInstance (index.js:189:9)
    at HTMLDivElement.buttonHandler (index.js:220:13)

Here is my most recent code… JS Bin - Collaborative JavaScript Debugging

1 Like

This needs to be verified. I only suggested it, not meant it for actual use.

`
        <tr id="${this.memberId}">
          <td><div class="name">${this.memberName}</div></td>
          <td><div class="checkIn"><button>Check In</button></div></td>
          <td><div class="checkOut"><button>Check Out</button></div></td>
          <td><div class="bool">${this.isCheckedIn}</div></td>
        </tr>
        `

In theory, the template can be a constant since the interpolated data fields are object attributes, so should be editable. Needs confirmation.

Otherwise, use let and rest easy.

let memberRow = ...;

document.getElementById("thead").innerHTML = headerRow;

When we think about it, all we need in the source HTML is,

<table id="roster"></table>

Below we take a step by step approach to generating and appending the <thead> element to this table.

const roster = document.querySelector('#roster');
let el = document.createElement('thead');
let tr = document.createElement('tr');
let td = document.createElement('td');
td.appendChild(document.createTextNode("Member"));
tr.appendChild(td);
td = document.createElement('td');
td.appendChild(document.createTextNode("Check In"));
tr.appendChild(td);
td = document.createElement('td');
td.appendChild(document.createTextNode("Check Out"));
tr.appendChild(td);
td = document.createElement('td');
td.appendChild(document.createTextNode("Checked In?"));
tr.appendChild(td);
el.appendChild(tr);
roster.appendChild(el);

https://repl.it/LjNS

It needs to be explored more fully on your end. Work with limited models, not a full blown program, when ironing out details and approaches. This is where we really get under the hood.

As for the problems in your newest iteration, there is a lot to iron out once you get the gist of the above demo and learn ways to optimize it. It will be time well spent.


I should add, the above is not intended for actual use. In practice, your source HTML should contain the THEAD (and TBODY) element already, rather than waiting for the script to append it. There is no dynamic content that would necessitate generation.

Howevr, the above model could well apply to the new table row, which is my intention in showing it to you. I’ll leave you to explore further.

3 Likes

I’ve been noticing that in most of your html, you put the <script> element after the parts of the body that the script deals with… should I be doing this as well? or is it okay to have it before the table?

1 Like

One thing we should set straight… Should. There is no should in programming. Advisable, recommended, preferred, … These are pretty much the guidelines. Should just doesn’t fit in. But, we use that word alot to convey some sort of preference or recommendation or guideline, just to be soft.

Trouble is we derail the main purpose of a guideline which is essentially the width of the highway upon which this idea is being driven. *should* often ignores *breadth*.

3 Likes

Well then, would you advise that I do this as well?

3 Likes

Clever…

Positioning of script is not based upon syntactical rules, but whether or not they will have any effect in the normal flow of the load sequence.

3 Likes

I guess the better question would be, which is more efficient in my situation?

1 Like