Skip to content
This repository has been archived by the owner on Feb 9, 2024. It is now read-only.

London 9 - Vitalina Kuzmenko - cyf-hotel-react #540

Open
wants to merge 59 commits into
base: master
Choose a base branch
from

Conversation

VitalinaKuzmenko
Copy link

@VitalinaKuzmenko VitalinaKuzmenko commented Feb 27, 2023

Copy link

@sampennington sampennington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is crazy good Vitalina!! I'm so impressed 😮
I love how much care you have put into it all, from the way you've written functions, to commit messages and taking error handling into account 🥳

On thing I did notice, there are quite a few instances of using let over const, where it's not wrong, its a lot more common to use const for variables we are not reassigning, did you do this intentionally?

@@ -14,6 +16,7 @@
"eject": "react-scripts eject"
},
"devDependencies": {
"eslint-plugin-react-hooks": "^4.6.0",
"husky": "^8.0.3",
"prettier": "^2.8.4",
"pretty-quick": "^3.1.3"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we we have our dependencies int he correct sections? 😃

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. I was installing dependencies using npm install. And it was installed automatically. Is something wrong with it?

Copy link

@sampennington sampennington Mar 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there is just one issue, eslint should be included in the dev dependencies section.

There are two types of dependencies:

  • "dependencies": Packages required by your application in production. (production is just the live website you deploy)
  • "devDependencies": Packages that are only needed for local development and testing.

Eslint falls into the second one

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I have it inside devDependencies, no?
"devDependencies": {
"eslint-plugin-react-hooks": "^4.6.0",

Maybe I will ask you personally about it :) Thank you!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants