Warfunk
commented
3 years ago Hey Steven, Thank you very much for your advice, insight, and encouragement. I look forward to reviewing your PR and suggestions. This was definitely above and beyond my expectations and is greatly appreciated. Cheers, Alex
On Sat, Jan 30, 2021 at 10:58 AM Steven Scaffidi notifications@github.com wrote:
Super clean code so really nice job overall. Here are some notes:
- src/SignInRegister/Fields.js you import { React, useState } from 'react'. React here would actually be undefined. React is the default export (you're trying to call it like a named export). So import React, { useState } from 'react' would work. Since you're using React 17 though, you don't need React in scope at all (which is why you don't get an error while running this code). You do the same thing in App.js. Same answer.
- In App.js line 93 you use a nested ternary. Typically this is frowned upon. In other works `const bool = foo ? 'bar' : somethingElse ? 'hi' : 'no'. Typically you want to avoid nested ternaries because they are messy and most Eslint configs will not let you (for example: https://airbnb.io/javascript/#comparison--nested-ternaries). You can rewrite what you have with a useMemo. I'll submit a PR so you can see what I'm talking about.
- On Welcome.js instead of returning a fragment you can either use && or you can return null. In my PR I'll show this.
- In App.js, you use 5 setStates which is fine. Try to force yourself from time to time to use useReducer instead. I'll do this in my PR too. For example, lines 24-27 will result in 4 rerenders. If you used a useReducer, it would result in 1. Typically it doesn't matter because components are small, but it is a nice performance optimization. Also useState is simply useReducer wrapped (if you look at the React source code).
- Get use to using react-router rather than using your own implementation with state https://reactrouter.com/. Most React apps will typically use react-router.
Overall great job. This is well beyond what I typically see from a junior programmer. Honestly, you should have 0 trouble getting a job. just go out there and apply.
— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Warfunk/mt-ski-resorts/issues/1, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQBBWJB3CRSKC4L6AUO4MU3S4QUDVANCNFSM4W2NTBKQ .
Super clean code so really nice job overall. Here are some notes:
import { React, useState } from 'react'. React here would actually be undefined. React is the default export (you're trying to call it like a named export). Soimport React, { useState } from 'react'would work. Since you're using React 17 though, you don't need React in scope at all (which is why you don't get an error while running this code). You do the same thing in App.js. Same answer.Overall great job. This is well beyond what I typically see from a junior programmer. Honestly, you should have 0 trouble getting a job. just go out there and apply.