Can you please review my Wikipedia Viewer project

Code Feedback
1

https://codepen.io/zreticulan/full/RLmKzV/

Thanks!

2

Your WIkipedia viewer is pretty cool and looks neat!

I just noticed a handful of repetition in your code. For example, you have 5 of these:

$("#item1").click(function(){ 
  $("#item1").toggleClass("bold");
  $("#snippet1").toggleClass("hidden");
});

The only thing that changes between those five are the indexes that range from 0 to 4. You can use a for-loop and an extra function definition to make it cleaner:

function itemClick(index) {
  return function() {
    $('#item' + index).toggleClass('bold');
    $('#snippet' + index).toggleClass('hidden');
  };
}

for (let i = 0; i < 5; i++) {
  $('#item' + i).click(itemClick(i));
}

I noticed that the first one has a second argument (the 5000) that’s passed to the second toggleClass call, but it seems to work fine without it.

You can do something similar with this part:

  $("#snippet0").addClass("hidden");
  $("#snippet1").addClass("hidden");
  $("#snippet2").addClass("hidden");
  $("#snippet3").addClass("hidden");
  $("#snippet4").addClass("hidden");
  
  $("#item0").html("").removeClass("bold");
  $("#item1").html("").removeClass("bold");
  $("#item2").html("").removeClass("bold");
  $("#item3").html("").removeClass("bold");
  $("#item4").html("").removeClass("bold");

and this part:

      $("#item0").html(x.query.search[0].title);
$("#snippet0").attr("href", "https://en.wikipedia.org/?curid=" + x.query.search[0].pageid).html(x.query.search[0].snippet + " ...");  
    
       $("#item1").html(x.query.search[1].title);
$("#snippet1").attr("href", "https://en.wikipedia.org/?curid=" + x.query.search[1].pageid).html(x.query.search[1].snippet + " ...");  
    
       $("#item2").html(x.query.search[2].title);
$("#snippet2").attr("href", "https://en.wikipedia.org/?curid=" + x.query.search[2].pageid).html(x.query.search[2].snippet + " ...");  
    
        $("#item3").html(x.query.search[3].title);
$("#snippet3").attr("href", "https://en.wikipedia.org/?curid=" + x.query.search[3].pageid).html(x.query.search[3].snippet + " ...");  
    
 $("#item4").html(x.query.search[4].title);
$("#snippet4").attr("href", "https://en.wikipedia.org/?curid=" + x.query.search[4].pageid).html(x.query.search[4].snippet + " ...");
1 Like
3

You’re right about the repetition. I’ve replaced two of such parts with the following self-invoking functions and for loops:

(function () {
for (var i = 0; i < 5; i++) {
$('#item' + i).removeClass('bold');
$('#snippet' + i).addClass('hidden');
} 
}) ();

and

(function () {
for (var i = 0; i < 5; i++) {
$("#item" + i).html(x.query.search[i].title);
$("#snippet" + i).attr("href", "https://en.wikipedia.org/?curid=" + x.query.search[i].pageid).html(x.query.search[i].snippet + " ...");  
}
}) ();

However, I’m not so sure about the first repeating block of code with the click event listeners attached to five individual items. Those items are meant to be clicked and emerge individually. Not sure how a for loop can help…

$("#item0").click(function(){ 
  $("#item0").toggleClass("bold");
  $("#snippet0").toggleClass("hidden");
});

$("#item1").click(function(){ 
  $("#item1").toggleClass("bold");
  $("#snippet1").toggleClass("hidden");
});

$("#item2").click(function(){ 
  $("#item2").toggleClass("bold");
  $("#snippet2").toggleClass("hidden");
});

$("#item3").click(function(){ 
  $("#item3").toggleClass("bold");
  $("#snippet3").toggleClass("hidden");
});

$("#item4").click(function(){ 
  $("#item4").toggleClass("bold");
  $("#snippet4").toggleClass("hidden");
});
4

The for loop is just for attaching event handlers for these items (in a more concise way). Their behavior shouldn’t change.

5

I like the minimalist look. Only recommendation I would make is to give the page snippets a hover effect (like turning blue or bold) so that it’s a little more intuitive that it’s a link.

1 Like
6

Yeah, it makes sense. Thanks!