tanaypratap / teamtanay.jobchallenge.dev

The web repository for all things #teamtanayjobchallenge
https://2020.teamtanay.jobchallenge.dev
132 stars 429 forks source link

[Project][Feedback][Code review] Please provide feedback for my project. #315

Closed mritunjaysaha closed 4 years ago

mritunjaysaha commented 4 years ago

Hey Reveiwer,

Could you please provide some feedback for this project. This is the first site that I built using JavaScript.

Github: https://github.com/mritunjaysaha/corona Website: https://covidupdate.netlify.com

Thanks.

anurag-majumdar commented 4 years ago

Hi @mritunjaysaha , your project is under review. Please expect to hear back in a day's time. Thanks for your patience.

anurag-majumdar commented 4 years ago

Hi @mritunjaysaha , your project looks really impressive. Here are the below points for review:

Product Thinking: Your thought process definitely stood out here as you created a project based on an alarming situation. This project will help to keep a track of the cases happening.

It would be great if you could add the following features if the API gives details accordingly:

  1. New cases every day in total and for each state if any?
  2. You can consider plotting a graph to show total number of case increase in entire India, no need for statewise changes in graphs.

You can take some inspiration from the following site too: https://www.covid19india.org/

UI/UX:

  1. The design of the application is quite clean. Kudos on making the entire site responsive!
  2. Good colour usage and UI components look great at their respective places.
  3. Just one tip, try making the following design on 1 row instead of 2 rows:

image

On making them appear in the same line will help reduce space and make the site look cleaner and more classy.

Code:

  1. Your code has good variable naming practices, it made me easier to read. Such well written code generally don't need a lot of comments and you have proved this by commenting on only important areas.
  2. Your code is well linted too, with good proper usage of spaces and tabs as well.
  3. You have used ES6 practices in your code at certain places. It will be really nice if you could replace "var" with let and const inside for loops also. You can also try async, await syntax instead of promise chaining.
  4. For performance, you can try using document.createDocumentFragment to create a fragment and then append all the data to that fragment inside the loop. After the loop is done, you can append the fragment back to the document.
  5. Try caching the variables outside for loop, for example:

var divContainer = document.getElementById("helpline-data");

The above line of code was used inside of a for loop in addHelpline method. Try caching the document element at the beginning.

Scalability & Architecture:

  1. Your project structure is quite simple here. It looks fine for that purpose.
  2. You have modularized code according to each responsibility with the use of functions such as "addData" and "addHelpline". If you want to further use this principle, try to create separate methods for creating a document item or for adding data to table.
  3. Your code is quite scalable. It handles the part of showing data in table accordingly to the data fetched from API call.

Enhancements: I have pointed out the requirements in each of the areas. You can work accordingly on all or some of the important parts. It will definitely boost some points.

Overall, your project is really quite solid with all the points in mind. Let me know when you are done making changes, will review those specific areas again. Happy learning! 😄

mritunjaysaha commented 4 years ago

Thank you sir. I'll be working on the requirements.

mritunjaysaha commented 4 years ago

Hi @anurag-majumdar sir,

I have completed making all the requirements in each of the areas. Could you please review the changes that I made. I have also added the feature to show new cases for both total cases and state-wise cases.

anurag-majumdar commented 4 years ago

@mritunjaysaha Sure. Thank you for your patience. Will get back to you asap.

anurag-majumdar commented 4 years ago

Hi @mritunjaysaha , great job on the new use cases. It's very easy to implement a project but to work on new use cases is something which really is an important part of everyday job.

I have gone through your code as well as the site. I guess I will be using your site to keep track of the latest corona virus cases. 😄

You have implemented the latest async, await syntax along with other ES6 features. I'm sure you found out the performance enhancement using createDocumentFragment syntax.

Also the responsive design changes and the entire feature of showing charts was very well done.

Just one tip, the charts look a bit big, you can try responsive design for the charts by including 3 of them in a single row in a large screen and accordingly provide area to each chart according to the content of the page.

Here at Job Challenge we respect students who have the ability to complete a given feature. You have really shined in this area!

@tanaypratap @gkanishk Please take a look at this project. I think we will be looking at Covid 19 stats from this site going forward.

mritunjaysaha commented 4 years ago

Hi @anurag-majumdar sir. Thank you so much.

I have made the charts responsive and now all the charts appear on a single row. I had to use javascript to make the charts resize, hence the screen size is assigned to the charts when the page loads.

gkanishk commented 4 years ago

Hey @mritunjaysaha , Now charts look much more aligned than before nice work! I tried to open that on my mobile device but there is some problem in mobile mode. image image You have used flex that is to be done same for mobile devices define flex inside the max-width media query you have defined and see if that works, I tried that works fine after adding.

mritunjaysaha commented 4 years ago

Hey @gkanishk, I have made the changes and the charts will load normally on mobile devices.

As you mentioned about flex, it did not seem to responsively resize the charts. So, I had to assign the chart size when the screen loads.

gkanishk commented 4 years ago

Works fine now 👍 @mritunjaysaha