rneha725 / Online-Judge-for-LITG

Project for Learn It, Girl 2016.
1 stars 0 forks source link

Review (Feb 17) #2

Open dshgna opened 8 years ago

dshgna commented 8 years ago

Great work so far Neha!

home.html It's better to use JQuery's $(document).ready() instead of JS onload(). document.ready() is fired as soon as possible while onload() is called after all the page's content are loaded. More here.

fetchProblems() Here, we have to iterate through an array of objects. In JS, for...in is meant to be used to iterate over properties of objects. To iterate over an array, it is best to use either the sequential for loop or the newly introduced for...of loop can be used. More on this here

Displaying problem data from JSON file

  1. Combine the problem.js files into a single JSON file. Using a JSON generator will make life easier ;)
  2. For each object, include an "id" key.
  3. JSON -> JS object conversion (http://api.jquery.com/jquery.getjson/)
  4. Modify loadPage() to display the relevant problem for a particular page based on id.

Fix indentation - use Codepen's Tidy to get this done quickly

rneha725 commented 8 years ago
dshgna commented 8 years ago

Really loving how you are taking the initiative!

  1. Ideally, only external JS should be included in HTML. $(document).ready(...) should be in the JS. For example:
$(document).ready(function() {
navigationBar();
});

Having $(document).ready(...) in both the js files included is not a problem.

  1. for...of may not be supported by your browser as its a recent construct, So a sequential for loop is perfectly fine here.
  2. Using a problemCodealso works perfectly.
  3. At the moment, you will need to have separate pages for the mock problems for testing, However, once we move into using React, the template will suffice.