add calculator component and basic styles #3
Conversation
There was a problem hiding this comment.
Hi @Tiago-Lelinski-Marin,
Good job so far!
There are some issues that you still need to work on to go to the next project but you are almost there!
Highlights
- The Pull Request has a proper title and description. ✔️
- You have added a descriptive Readme file. Good Job. ✔️
Required Changes ♻️
Check the comments under the review.
Optional suggestions
Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.
You can also consider:
- Please make sure you right descriptive comments. Commenting involves placing Human Readable Descriptions inside of computer programs detailing what the Code is doing. Proper use of commenting can make code maintenance much easier, as well as helping make finding bugs faster. Further, commenting is very important when writing functions that other people will use. ❌
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
src/App.js
Outdated
| @@ -1,29 +1,14 @@ | |||
| import logo from './logo.svg'; | |||
| /* eslint-disable react/prefer-stateless-function */ | |||
There was a problem hiding this comment.
- Let's make sure we don't use disable linter rules. These are here to help us write clean code.
There was a problem hiding this comment.
I disable because the error the linter rule was telling me to create a purê funcional component, and the requirement asks for another thing.
src/components/Calculator.js
Outdated
| @@ -0,0 +1,36 @@ | |||
| /* eslint-disable react/prefer-stateless-function */ | |||
There was a problem hiding this comment.
- Let's make sure we don't use disable linter rules. These are here to help us write clean code.
src/App.css
Outdated
| from { | ||
| transform: rotate(0deg); | ||
| } | ||
| #calculator :nth-child(5):hover { |
There was a problem hiding this comment.
- Let's make sure we don't have used ids as selectors.
src/App.css
Outdated
| background-color: rgb(255, 195, 85); | ||
| } | ||
|
|
||
| #input { |
There was a problem hiding this comment.
- Let's make sure we don't have used ids as selectors.
src/App.css
Outdated
| #calculator :nth-child(5), | ||
| #calculator :nth-child(9), | ||
| #calculator :nth-child(13), | ||
| #calculator :nth-child(17), | ||
| #calculator :nth-child(20) { |
There was a problem hiding this comment.
- Let's make sure we don't have used ids as selectors.
src/App.css
Outdated
| margin-top: 20vh; | ||
| } | ||
|
|
||
| #calculator { |
There was a problem hiding this comment.
- Let's make sure we don't have used ids as selectors.
src/App.css
Outdated
| width: 50%; | ||
| } | ||
|
|
||
| #calculator :nth-child(18) { |
There was a problem hiding this comment.
- Let's make sure we don't have used ids as selectors.
src/App.css
Outdated
| font-size: 1.2rem; | ||
| } | ||
|
|
||
| #calculator-placeholder { |
There was a problem hiding this comment.
- Let's make sure we don't have used ids as selectors.
There was a problem hiding this comment.
Hi there, how are you @arslanbisharat ?
Could you explain a little more about this? Why we shouldn't use ids?
Thanks a lot, for your time so far!
There was a problem hiding this comment.
Hello @Tiago-Lelinski-Marin 👋,
This is an additional review as a result of the second opinion request.
- Since using class-based components is the requirement of this milestone, You can suppress linter errors related to this.
- When you use the ID selector, it also creates a Javascript variable. This can cause hard-to-detect errors in the application. For this reason, it is recommended not to use the ID selector. But since this is not part of the project requirements or best practices, this should be requested as [OPTIONAL].
As this was the only reason that your project was not approved it will be approved as soon as possible.
Please ask for another review in your Dashboard and the next Code Reviewer will mark it as approved right away :)
Keep rocking 🚀!
|
Hello @sinansevgi how are you? Thanks a lot for clarifying all this, and thanks for your time. I really appreciate it! |
There was a problem hiding this comment.
Hi @Tiago-Lelinski-Marin,
Congratulations! 🎉
Your project is complete! There is nothing else to say other than... it's time to merge it 🚀 :
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
In this pull request I:
✔️ Deleted all the boilerplate from CRA (text, images, styles).
✔️ Created a directory called components.
✔️ Inside components, created a new Calculator.js file.
✔️ In Calculator.js, created a React component that matches the wireframe:
✔️ Used Use class based components to render the calculator