How can i improve the efficiency of targeting my html elements using Javascript

Question

I have a menu with links and i have them toggle their hr's opacity to 1 when i click on each link, of course making all other links (hrs) on the menu back to the original opacity of 0 which they previously had (Classic menu UX).

The problem is to achieve this i had to use 2 nested for loops using JS. From what i learned about Big O notation, that isn't very effiecient, especially that with jquery all this code is done in a single line. My question is how can i improve the efficiency of this code with Vanilla JS only?

HTML


    Home
    Movies
    TV Shows
    Documentaries
    Favorites
    Collection

JS

const menulink = document.querySelectorAll('.menu h6')
for(let item of menulink) {
  item.onclick = () => {
    for(let i=0;i

user_2100126 · Answer

I think you can achieve this saving current item:

const menulink = document.querySelectorAll('.menu h6');
let current = null;

for(let item of menulink) {
  item.onclick = () => {
    if (current) current.style.opacity = '0'
    current = item.querySelector('hr')
    current.style.opacity = '1'
  } 
}

hr {
  opacity: 0;
}


    Home
    Movies
    TV Shows
    Documentaries
    Favorites
    Collection

user_2684106 · Answer

Behold!

const menulink = document.querySelectorAll('.menu h6')
let activeHr;
for(let item of menulink) {
  item.onclick = (event) => {
    if (activeHr) {
        activeHr.style.opacity = '0';
    }
    
    activeHr = event.currentTarget.querySelector('hr');
    activeHr.style.opacity = '1';
  } 
}

hr {
opacity: 0;
}


    Home
    Movies
    TV Shows
    Documentaries
    Favorites
    Collection

user_5867572 · Answer

All the above answers are trying to make your code work - rather than considering the best html and CSS - the use of the hr is unnecessary - all you need is to set the active class on the element and have CSS to apply a border-bottom to it.

So that the headings do not jump on the click - I have added a transparent border under each h6 and then when you click it applies the active class and the styling simply colors the bottom border.

You should not use a html element (hr/) for styling purposes - IMO.

I also disagree with the use of h6's here - these o not appear to be headings.... but I have left it incase there is more to the code than you are showing- eg if they are headings running down the page - but a regualr navigation list would seem to be more appropriate here.

Thanks to @Barmar for the skeleton of the code I have used and I agree with his approach of using the active class. Note that I am using a null check for the removal of the existing active class -although that could be alleviated by simply having the first heading set to active from the start.

const menuLinks = document.querySelectorAll('.menu h6')

for (let menuLink of menuLinks) {
  menuLink.onclick = () => {
    document.querySelector('.menu h6.active')?.classList.remove('active');
    menuLink.classList.add('active');
  }
}

.menu h6 {
  padding-bottom: 2px;
  border-bottom: solid 1px transparent;
  transition: all 0.2s ease-in-out
}


.menu h6.active {
  border-bottom-color: #000
}

.menu h6:hover {
  border-bottom-color: #000;
  transition: all 0.25s ease-in-out
}


  Home
  Movies
  TV Shows
  Documentaries
  Favorites
  Collection

user_13504647 · Answer

You can use hover option while styling your heading tags.In hover you can add color of your choice so that whenever you hover on it color will change

user_1491895 · Answer

Don't set the style directly, do it through the style of a CSS class. Then you can find the element that currently has the class and remove it, while adding the class to the newly selected element.

const menulink = document.querySelectorAll('.menu h6')
for (let item of menulink) {
  item.onclick = () => {
    let old = document.querySelector('.menu h6 hr.active');
    if (old) {
      old.classList.remove("active");
    }
    item.querySelector('hr').classList.add("active");
  }
}

.menu h6 hr.active {
  opacity: 1;
}

.menu h6 hr {
  opacity: 0;
}


  Home
    
  
  Movies
    
  
  TV Shows
    
  
  Documentaries
    
  
  Favorites
    
  
  Collection

user_13816862 · Answer

I would first use instead of h6s if its a navbar and for the amount of links that code ain't inefficient! Cheers mate.

How can i improve the efficiency of targeting my html elements using Javascript

6 Answers