Twitch.tv project (feedback)

Twitch.tv project (feedback)
0

#1

Hello,

I just completed the Twitch TV Project.

Project Link - https://codepen.io/X140hu4/full/KyaZvX/

I had a lot of trouble to make it work… but it was interesting to work with multiple requests firing at the same time. I am going to look at other people’s code now in order to see how they overcame this.

So far in my projects I have avoided using jquery or any framework.

I wondered if there was a better way in pure JS to dynamically create nodes?

Thanks for taking the time to look at my code and give feedback :wink:


#2

I would strongly suggest refactoring the following code.

// Display all channels and its arrow
document.getElementById('btn_all').addEventListener('click', function() {
  for(var i = 0, n = stream_on.length; i < n; i++) {
    stream_on[i].style.display = "block";
  }
  for(var i = 0, n = stream_off.length; i < n; i++) {
    stream_off[i].style.display = "block";
  }
  displayArrow(0);
});
// Display online channels and its arrow
document.getElementById('btn_online').addEventListener('click', function() {
  for(var i = 0, n = stream_on.length; i < n; i++) {
    stream_on[i].style.display = "block";
  }
  for(var i = 0, n = stream_off.length; i < n; i++) {
    stream_off[i].style.display = "none";
  }
  displayArrow(1);
});
// Display offline channels and its arrow
document.getElementById('btn_offline').addEventListener('click', function() {
  for(var i = 0, n = stream_on.length; i < n; i++) {
    stream_on[i].style.display = "none";
  }
  for(var i = 0, n = stream_off.length; i < n; i++) {
    stream_off[i].style.display = "block";
  }
  displayArrow(2);
});

function displayArrow(element) {
  for (var i = 0, n = arrows.length; i < n; i++) {
    var disp = "none"
    if (i === element) {
      disp = "block"
    }
    arrows[i].style.display = disp;
  }
}

I noticed the call to displayArrow is not even needed (I commented out displayArrow(0), displayArrow(1), displayArrow(2) and the entire displayArrow function definition and it “seems” the code still works the same. My suggestion is to create a standalone function (could be called displayStreams) and replace the repetitive sections above and have the for loops be in this new function . Then this new function would just need two parameters (let’s call them on and off) which would contain either “block” or “none” depending on which button was clicked.

I will let you figure out how to write the new function, but I will show you how your addEventListener callback functions could look:

// Display all channels and its arrow
document.getElementById('btn_all').addEventListener('click', function() {
  displayIt("block","block");
});
// Display online channels and its arrow
document.getElementById('btn_online').addEventListener('click', function() {
  displayIt("block","none")
});
// Display offline channels and its arrow
document.getElementById('btn_offline').addEventListener('click', function() {
  displayIt("none","block")
});

#3

Hi @RandellDawson!

Thank you very much for reviewing my code.

Deleting the displayArrow(n) does not work. The purpose is to show the arrow below the buttons [All], [Online] and [Offline], so if you take them out, run and click on Online or Offline, the arrow remains under “All”.

I have added the following function:

function displayIt(onDisplay, offDisplay) {
  for(var i = 0, n = stream_on.length; i < n; i++) {
    stream_on[i].style.display = onDisplay;
  }
  for(var i = 0, n = stream_off.length; i < n; i++) {
    stream_off[i].style.display = offDisplay;
  }
}

#4

OK, I guess I was not paying attention to what that function was actually doing. Now that I see what it does, I would do the following to only call displayArrow one time inside displayIt like below. I added some comments to explain the changes.

// Display all channels and its arrow
document.getElementById('btn_all').addEventListener('click', function() {
  displayIt('block','block','all');
});
// Display online channels and its arrow
document.getElementById('btn_online').addEventListener('click', function() {
  displayIt('block','none','online');
});
// Display offline channels and its arrow
document.getElementById('btn_offline').addEventListener('click', function() {
  displayIt('none','block','offline');
});

function displayArrow(element) {
  for (var i = 0, n = arrows.length; i < n; i++) {
    var disp = "none"
    if (arrows[i].id === element) { // note the use of arrows[i].id because element is text now
      disp = "block"
    }
    arrows[i].style.display = disp;
  }
}

function displayIt(onDisplay, offDisplay,status) {
  for(var i = 0, n = stream_on.length; i < n; i++) {
    stream_on[i].style.display = onDisplay;
  }
  for(var i = 0, n = stream_off.length; i < n; i++) {
    stream_off[i].style.display = offDisplay;
  }
  displayArrow(status) // caledl one time
}

EDIT: I forgot to mention that the above code would also need you to add an id to each button as shown below:

      <ul id="arrows">
        <li>
          <div class="arrow-up" id="all"></div>
        </li><!--
     --><li>
          <div class="arrow-up" id="online"></div>
        </li><!--
     --><li>
          <div class="arrow-up" id="offline"></div>
        </li>
      </ul>

#5

OK, I promise this is the last suggestion. You could simplify the displayArrow function further by:

function displayArrow(element) {
  for (var arrow of arrows) {
     arrow.style.display = "none";
  }
  document.getElementById(element).style.display = "block";
}

#6

Oh perfect! I wanted to use a differ for syntax but couldn’t make it work before :slight_smile: Thank you very much!