Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code Review - Week 2 #67

Open
VatsKan opened this issue Oct 9, 2020 · 0 comments
Open

Code Review - Week 2 #67

VatsKan opened this issue Oct 9, 2020 · 0 comments

Comments

@VatsKan
Copy link

VatsKan commented Oct 9, 2020

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant