1
\$\begingroup\$

I have finished the assignment but would love some feedback. Data.json contains news articles data.

Requirements for assignment:

Fetch the data and render it on the page. Initially the page should show only a list of titles fetched from the source. On clicking the title it should show the relevant image and content of the article associated with the title as well as related stories, if available. There should be a “read more” link to the article source. Above the list of articles there should be a search field for filtering the articles. Upon typing either the title or a keyword from the article content the list should be filtered to show only relevant results from an already rendered content. The filtering should occur as the user is typing. Additionally the user should be able to filter by date published.

Restrictions:

You have one week to complete this task. You are not allowed to use jQuery or any other libraries, including helper libraries such as templating ones. Instead, use core Javascript features. You can use either ECMAScript5 or ECMAScript6 (ES2015) features. Code should be clean, modular and reusable.

var xhr = new XMLHttpRequest();

xhr.open("GET", "data.json");
xhr.onreadystatechange = function() {
  if (this.status === 200 && this.readyState === 4) {
    var responseParse = JSON.parse(this.responseText);
    var articles = responseParse.results;
    //sort articles by date published
    var filterByLatestDate = document.getElementById('filterByLatestDate');
    var filterByOldestDate = document.getElementById('filterByOldestDate');
    filterByOldestDate.addEventListener("click", function() {
      articles.sort(function(a, b){
        return new Date(a.publishedDate) - new Date(b.publishedDate);
      })
      addDOM(articles);
    })
    filterByLatestDate.addEventListener("click", function() {
      articles.sort(function(a, b){
        return new Date(b.publishedDate) - new Date(a.publishedDate);
      })
      addDOM(articles);
    })

    addDOM(articles);

  }
};

xhr.send();
//adding DOM element when data recieved
var addDOM = function(data) {
  console.log(data);
  var articleList = '<ul>';
  data.forEach(function(article,index){
    var relatedStories = article.relatedStories;
    articleList += '<li id="article" onclick= "basanti('+index+')">';
    articleList += '<input type="checkbox" name="article-heading" id="article-heading'+index+'"><label class="title" for="article-heading'+index+'">'+ article.titleNoFormatting +'</label>';
    articleList += '<div id="article-detail'+index+'" style="display:none">';
    articleList += '<img src="'+article.image.url+'" alt="'+ article.titleNoFormatting +'" style="height:'+article.image.tbHeight+';width:'+article.image.tbWidth+';" />';
    articleList += '<p class="content">'+ article.content+'</p>';
    articleList += '<a href="'+article.unescapedUrl+'"> Read more.. </a>';
    if(relatedStories){
      relatedStories.forEach(function(articleRelatedStories){
        articleList += '<ul id="related-stories">';
        articleList += '<li><a href="'+articleRelatedStories.unescapedUrl+'">'+articleRelatedStories.titleNoFormatting+'</a></li>';
        articleList += '</ul>';
      });
    };
    articleList += '</div>';
    articleList += '</li>';
  })
  articleList += '</ul>';

  document.getElementById('article-list').innerHTML = articleList;
}
//checkbox when clicked on title the details will open
var basanti = function (index){
  var articleDetail = document.getElementById('article-detail'+index+'');
  var articleHeading = document.getElementById('article-heading'+index+'');
  if(articleHeading.checked){
    articleDetail.style.display = 'block';
  }else {
    articleDetail.style.display = 'none';
  }
}
//filtering by title and content
var filterByContentTitle = function() {
  var searchNode = document.getElementById('searchFilter')
      searchText = searchNode.value,
      titles = document.getElementsByClassName('title'),
      contents = document.getElementsByClassName('content')
      regexp = new RegExp(searchText,'i');
  for (var i = 0; i < titles.length; i++) {
     var title = titles[i];
     var content = contents[i];
     (title.textContent.search(regexp) < 0 && content.textContent.search(regexp) < 0 ) ?
       title.parentNode.style.display = 'none':
       title.parentNode.style.display = 'block';
   }
}

//html
<body>
    <input type="search" id="searchFilter" onkeyup="filterByContentTitle()" />
    <input type="radio" id="filterByLatestDate" name="filterByDate" />Filter By Latest Date
    <input type="radio" id="filterByOldestDate" name="filterByDate" />Filter By OldestDate Date
        <div id="article-list"></div>
    <script type="text/javascript" src="app.js"></script>
  </body>
\$\endgroup\$
  • \$\begingroup\$ Hi, welcome to Code Review. I hope you receive great answers! \$\endgroup\$ – Tunaki Apr 17 '16 at 14:51
1
\$\begingroup\$
;(function(){
  // ALL YOUR CODE HERE
}());

Consider putting your code inside a closure. That way, you don't put everything in the global scope, where it will easily be overidden by other code.

var basanti = function (index){

This is hard to debug because they're nameless functions assigned to variables. They become hard to trace in the stack. Prefer named functions/function declarations instead of this method of defining functions.

var xhr = new XMLHttpRequest();

Use fetch instead of this old API. It's the closest you can get to jQuery-like AJAX operations. It should be present in Chrome and Firefox.

var addDOM = function(data) {
  console.log(data);

While console.log is a handy way to debug, the problem with it is that you'll often forget to remove them when it's time to deploy. Consider using the dev tools when debugging. Use breakpoints and use the scope pane to inspect values.

articleList += '<li id="article" onclick= "basanti('+index+')">';
articleList += '<input type="checkbox" name="article-heading" id="article-heading'+index+'"><label class="title" for="article-heading'+index+'">'+ article.titleNoFormatting +'</label>';

Use template strings instead of concatenating regular strings. One advantage of template strings is that you can interpolate values without concatenating, and you can also multi-line. Also present in Chrome and Firefox.

Additionally, consider splitting this operation into functions. Create a function that builds the list, the list items, list content. It won't change how it is structured, but it will avoid making addDom very bloated.

onclick= "basanti('+index+')"

Consider using addEventListener. This way, you don't use inline scripts as well as providing a global handler. To avoid having a handler per element, consider using event delegation via bubbling.

if(articleHeading.checked){
  articleDetail.style.display = 'block';
}else {
  articleDetail.style.display = 'none';
}

Instead of using inline styles to toggle visibility, use CSS. Define classes that show and hide an element. Then append/remove these classes on the element. The one problem with inline styles is that they can only be overridden by an !important in the CSS or replacing the value. This makes it hard for theming without writing terrible CSS and JS hacks.

for (var i = 0; i < titles.length; i++) {
  var title = titles[i];
  var content = contents[i];

titles isn't an array, but you can turn it into one by using array.slice like Array.prototype.slice.call(titles). Then you can use array methods like forEach to loop through the titles.

You are not allowed to use jQuery or any other libraries, including helper libraries such as templating ones.

Funny assignment.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.