Javascript tabs page outdated (uses deprecated event)

[MegaMech]

The how to make javascript tabs page is outdated and uses deprecated "event" in onclick. Consider altering to the following:

function openCity(evt, cityName) {

->         // e better explains that the required parameter is an element rather than an event (Or el, element, etc.).

function openCity(e, cityName) {

onclick="openCity(event, 'London')

->

onclick="openCity(this, 'London')

 evt.currentTarget.className += " active";

->

 e.className += " active";

 

 

https://www.w3schools.com/howto/howto_js_tabs.asp

[Ingolme]

If "event" actually is deprecated then by what means can you get information about the event inside the attribute?

Personally, I think the bigger problem is using HTML attributes to attach Javascript events in the first place.

[MegaMech]

Quote

If "event" actually is deprecated

Certainly a strange approach to question its deprecation without looking it up yourself. Not only is the use of event deprecated, but it was never a standard in the first place. See:

https://stackoverflow.com/questions/58341832/event-is-deprecated-what-should-be-used-instead

Implementing 'this' in its place is a great alternative because it returns the element which is sufficient for the tutorial.

 

Quote

problem is using HTML attributes

That's outside the scope of the tutorial as the subject is not "how to use javascript events." Sometimes the simple solution is the best one and in this tutorial using the html attribute is the best way to do it. It's the cleanest and the easiest method for beginners to comprehend and implement. Even the MDN does not provide a convincing argument regarding HTML attribute event avoidance for single-use or simple constructions.

If the tutorial were to use events it would have to look something like the following which would detract from the tutorials purpose; tabs.

These are just my thoughts on the matter. I believe in the tutorials current state users will have issues with browsers not properly returning `e` for e.currentTarget.className (In firefox it was returning null for me). I also don't see all the tutorials that use somewhat outdated implementations being updated which I take as an intentional part of w3schools methodology and approach to web-education.

 

// Hungarian notation style
const cTabButtons = document.querySelectorAll('.tablinks');

cTabButtons.forEach( (tabButton) => {
  tabButton.onclick = openCity(e, "London");
});

function openCity(e, cityName) {
  // Declare all variables
  var i, tabcontent, tablinks;

  // Get all elements with class="tabcontent" and hide them
  tabcontent = document.getElementsByClassName("tabcontent");
  for (i = 0; i < tabcontent.length; i++) {
    tabcontent[i].style.display = "none";
  }

  // Get all elements with class="tablinks" and remove the class "active"
  tablinks = document.getElementsByClassName("tablinks");
  for (i = 0; i < tablinks.length; i++) {
    tablinks[i].className = tablinks[i].className.replace(" active", "");
  }

  // Show the current tab, and add an "active" class to the button that opened the tab
  document.getElementById(cityName).style.display = "block";
  e.currentTarget.className += " active";
}

This method still requires checking the id of e or adding a dataset to each element so the script knows which tab to open or an array. It certainly is not as user friendly with this method.

 

Edited April 26, 2021 by MegaMech

[dsonesuk]

The reason it should be done that way is because it should be separate from html, if you need to add another argument to inline html attribute, it means going through each html element using that attribute and changing it! Whereas you would just have to amend possibly a single line of code in a js file. It is really bad practice to add as a inline html attribute.

You can't just say 'event' is deprecated, as JavaScript can't function without an event triggering. It is how its used is the question.  Its not an element, its the current event ATTACHED to a element that triggered the function. 'this' refers to the element object that ran the function that was triggered by the event. THAT! should be referred as 'elem', 'el' if anything.

[MegaMech]

I believe this ends in semantics.

window.onload = () => {
  		tabButtons[0].onclick = openCity(e, "London");
		tabButtons[0].onclick = openCity(e, "York");
		tabButtons[0].onclick = openCity(e, "Ontario");
}

 

<div class="tab">
  <button class="tablinks" onclick="openCity(event, 'London')">London</button>
  <button class="tablinks" onclick="openCity(event, 'Paris')">Paris</button>
  <button class="tablinks" onclick="openCity(event, 'Tokyo')">Tokyo</button>
</div>

 

My example in the previous post requires cleverness to open the correct tab making the code more complex. Or removing the for each and manually creating three events. In-which case future improvements require changing each-line regardless if it's in html or js. For tabs I see the javascript version as unnecessary extra code and complexity. I look at tabs and I know what the code is going to be. It doesn't effect me from a coder perspective if it's in html or JS.

"should be separate from html & really bad practice." This is not the case. I agree code is cleaner, easier to read, and it's best practice. However, just like 'goto' has legitimate situations for its use so does inline-events. Not every use results in hundreds of instances. This isn't a massive database. It's a tab menu and whichever method a person decides to implement will likely be straight forward.

The global event object can be avoided instead using the addEventHandler being a proper solution

[Ingolme]

Your Javascript won't work because you're calling openCity() and assigning the return value of the function as the event handler, it seems you have little experience with using event listeners outside of HTML. Aside from that, using properties such as "onclick" for events is also a bad practice since it will overwrite any existing event listener on the element and can be overridden by other code written in the same way. This is why addEventListener() should be used to attach events.

If you're going to use HTML attributes for event listeners, the only way to access information about the event is through the "event" variable, which is passed into the attribute along with "this". All browsers support it and have to continue supporting it as along as HTML attributes are being used for events.

[dsonesuk]

You have all the information required from element itself, without the need of inline event attribute. You just need to know how to gain that information.

                                      // Hungarian notation style
                                      const
                                      cTabButtons = document.querySelectorAll('.tablinks');

                                      cTabButtons.forEach(tabButton => tabButton.addEventListener("click", function() {
                                      openCity(event, this.textContent);
                                      }));

 

[Ingolme]

2 hours ago, dsonesuk said:

You have all the information required from element itself, without the need of inline event attribute. You just need to know how to gain that information.

                                      // Hungarian notation style
                                      const
                                      cTabButtons = document.querySelectorAll('.tablinks');

                                      cTabButtons.forEach(tabButton => tabButton.addEventListener("click", function() {
                                      openCity(event, this.textContent);
                                      }));

 

As shown in this post, there isn't a good reason to use HTML attributes for events and they severely hinder code maintainability.

In this scenario though, the global event variable wouldn't be needed since addEventListener passes it into the callback. I'd rewrite it like this:

cTabButtons.forEach(tabButton => tabButton.addEventListener("click", function(e) {
  openCity(e, this.textContent);
}));

 

Edited April 28, 2021 by Ingolme