Skip to content

Code Review - Week 2 #67

@VatsKan

Description

@VatsKan

First of all, I want to congratulate you all. You've done an amazing job and its amazing that you managed to learn and implement so many new technologies including firebase in 2 weeks! It's been a delight mentoring this dope team, and you all seemed to get along and work together so well which is inspiring. And have really enjoyed popping into your room and hanging out with you sweet cherry pickles.

  • whoop whoop - there's a readme! and its good :). you may want to add to the local setup, how to locally run the cloud functions.
  • nice you managed to get in quite a few tests! :)
  • remove unused files - e.g. getUsers.js
  • your helper.js functions look insanely cool! im not sure what they do and as they may be a little hard to read. Although i generally advise not to have too many comments in the code and make the code as readable as possible instead; in this case it may be worth adding a small comment above each one to explain what it does, or think about how you can name the functions better so that it is more self explanitory (which may be better).
  • Would also look at code review from last week if you have time to make some of those changes. e.g. i would definitely put the return of the array in getMedicationDB in the previous .then(). Also much nicer organisation of code from last week, would still look at consistent file naming conventions (e.g. perhaps for any react component, Capatilise file name and use camel case).
  • DailyViewArray.js has nested react components. might make it more readable if you're able to seperate these components out, and perhaps pass the props you need down into the child.
  • onMouseEnter={() => setInputType(currentType => "text")} "currentType is declared but its value is never read". can just replace currentType with (). (on signup.js). Should remove any unused variables throughout project to make code cleaner...eslint should be configured to pick these up for you.
  • in login.js you have empty setError();, does this work? you may need to put in any empty string here setError('');?
  • you have a PageNotFound.jsx file, I think it needs to be a js file? but im not familar with what .jsx does? or are all the components supposed to be jsx files?
  • you may also want to move your cypress test directly into the 'integration' folder and out of the 'integration/examples' folder, as it may be confusing.
  • nice job writing cloud functions. I think you can remove the .catch in the getNHSData function so that the error can get passed to the next() when you use it in your cloud function. Also im not sure if next actually works with cloud functions (i don't know why it wouldn't), but if it does that's cool.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions