Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
You've done a great job implementing the core logic for calculating and displaying the population statistics! I am requesting a couple of changes to fully meet the project requirements.
First, the task requires that changes be made only to the src/scripts/main.js file. Please revert the modifications made to the .scss style files.
Additionally, the requirement to "Make sure that the given string can be converted to a number" is not fully implemented. Your current code converts the string but doesn't check if the result is a valid number. If the text were non-numeric, this would produce NaN and lead to incorrect calculations. You can fix this by adding a check, for example using !isNaN(), to ensure you only add valid numbers to your array.
These are important checks for making your code more robust. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
src/scripts/main.js
Outdated
| const number = Number(clean); | ||
|
|
||
| numbers.push(number); |
There was a problem hiding this comment.
The task requires you to 'Make sure that the given string can be converted to a number'. Your current code attempts the conversion but doesn't verify the result. If pop.textContent contained a non-numeric string, Number(clean) would result in NaN, which would then be pushed into the numbers array and break the total and average calculations. You should add a check here to ensure only valid numbers are added to the array.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the feedback on the JavaScript logic! Your code now correctly handles number validation, and the calculations are working perfectly. However, this requires changes because files other than src/scripts/main.js have been modified. The task requirements specifically state that only the main.js file should be changed. I've noticed modifications in:
src/styles/_fonts.scsssrc/styles/main.scss
Please revert the changes in these style files. Once that's done, your solution will be ready for approval. You're very close!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| src: url('../fonts/Roboto-Regular-webfont.woff') format('woff'); | ||
| font-weight: normal; | ||
| font-style: normal; | ||
| src: url('../fonts/Roboto-Regular-webfont.woff') format('woff'); |
There was a problem hiding this comment.
The task requirements specify that only the main.js file should be modified. Please revert any changes made to this .scss file to meet the project constraints.
|
|
||
| body { | ||
| background: #eee; | ||
| counter-reset: section; |
There was a problem hiding this comment.
Per the task instructions, only the main.js file should be changed. Please revert any modifications made to this stylesheet.
2 participants
No description provided.