Code review #1
Code review #1
Conversation
…Adding template checkbox
There was a problem hiding this comment.
First code review
NOTE: Sometimes, when the same issue appears duplicated, not always a comment has been made. That shouldn't prevent the issue from being fixed. E.g: arrow function parentheses.
A few tasks and a challenge one to be done after the review has been taken into account:
Tasks:
- Use react-hooks as mentioned in the review.
- Port the styles of the app to
emotion.
Challenge:
- Port the code of the app to use https://github.com/Drawbotics/fnfy
NOTE 2: Please, ask questions, we're not in the examining phase anymore, now we're in the learning phase ;)
| @@ -0,0 +1,33 @@ | |||
| { | |||
| "name": "groceries-app-react", | |||
There was a problem hiding this comment.
We use spaces instead of tabs for indenting code. Specifically, two spaces.
| @@ -0,0 +1,18 @@ | |||
| <!DOCTYPE html> | |||
| <html lang="en"> | |||
|
|
|||
| const Button = ({ children, secondary, ...props }) => { | ||
| const buttonClass = secondary ? styles.secondary : styles.button; | ||
| return ( | ||
| <> |
There was a problem hiding this comment.
Use <Fragment></Fragment> instead of <></>. That way it looks more consistent with the rest of the code.
| }; | ||
|
|
||
| Button.defaultProps = { | ||
| secondary: null, |
There was a problem hiding this comment.
Only define defaultProps when they're needed. In this case, setting secondary to null doesn't change anything. Treat defaultProps as a way of telling the users, "Hey, I really need a value for this in order to work correctly"
Also, defaultProps should correctly reflect the expected type according to propTypes. In this case, a boolean.
| font-weight: 500; | ||
| background: rgb(0, 83, 135); | ||
| border: 2px solid white; | ||
| color: white; |
| const grocery = this.groceryService.addGroceryItem(newItem); | ||
|
|
||
| this.setState({ | ||
| grocery: grocery, |
There was a problem hiding this comment.
Redundant same key: value. Replace it by this.setState({ grocery })
|
|
||
| render() { | ||
| const { isModalOpen, formType, itemUnderEdition } = this.state; | ||
| const contextElements = { |
There was a problem hiding this comment.
It's cool you considered you don't need Redux, but instead of having a huge AppContext have smaller context that would manage each pice of state separately. E.g: ItemsContext, UIContext, etc... and components would use the one they specifically need.
| }; | ||
|
|
||
| render() { | ||
| const { isModalOpen, formType, itemUnderEdition } = this.state; |
| import styles from './StatisticsView.module.scss'; | ||
|
|
||
| const StatisticsView = () => ( | ||
| <> |
| @@ -0,0 +1,10690 @@ | |||
| # THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY. | |||
There was a problem hiding this comment.
@larsbs I used both for various projects and yarn seems to be better in a way, that it is caching a lot of libraries, which makes it work faster. Do you have any information that might be interesting for comparison? At company we are using npm?
There was a problem hiding this comment.
Yes, at company we use npm. And there are various benefits for using npm over yarn (starting from version 5, that's the important thing):
- Independent tool (yarn is basically backed by fb)
- Comes with node and you can expect to find it in every other system
- The newer versions of
npmalso include a lock file basically removing the biggest different in favor ofyarn - Better support for compiling native libraries (this was the main reason that made us move away)
- Using
npm ciintead ofnpm i, the speed difference betweenyarnandnpmis basically none now. - And a few more I don't want to continue listing like checking dependencies for vulnerabilities, easier upgrade of dependencies, etc.
yarn used to be "better" than npm but not anymore, so the main gist is, why learn another tool when the default one already does everything?
I hope that helped you
There was a problem hiding this comment.
@larsbs Thank you! This is really valuable knowledge! I will read more about changes of the npm itself, but as I have a lot of tasks now, it will be added to my personal backlog :-)
No description provided.