add task solution#2146
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job implementing the core logic in main.js to calculate and display the population data! The use of toLocaleString() for formatting is a nice touch.
However, a couple of changes are needed before this can be approved:
- The task instructions explicitly state that only the
src/scripts/main.jsfile should be changed. Your submission includes modifications to style files (src/styles/_fonts.scssandsrc/styles/main.scss). Please revert the changes in all other files. - In
src/scripts/main.js, the calculation works for the current data, but it doesn't validate the input. If a<span>contained non-numeric text, the calculation would result inNaN. To fully meet the requirement to 'Make sure that the given string can be converted to a number', please add a check, for instance withisNaN(), to handle such cases and make your code more robust.
Once you've addressed these points, your solution will be ready!
✨ 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
| let total = 0; | ||
|
|
||
| populations.forEach( | ||
| (population) => (total += Number(population.textContent.split(',').join(''))), |
There was a problem hiding this comment.
This calculation works for the current data. However, requirement #2 asks you to 'Make sure that the given string can be converted to a number'. Your current code doesn't explicitly check if the conversion is successful. Consider what might happen if a <span> contained non-numeric text. Adding a check, for instance with isNaN(), would make your code more robust and fully meet the requirement.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on updating the JavaScript logic! The addition of the isNaN check is an excellent improvement and makes the calculation much more robust. However, this submission requires a change because it modifies files other than the allowed src/scripts/main.js. A critical requirement for this task is that only the main.js file should be changed. Your submission includes modifications to src/styles/main.scss. Please revert the changes in that file to proceed. Once you've done that, your solution will be ready for approval!
✨ 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
| margin: 32px 0; | ||
| list-style: none; | ||
| padding: 0; | ||
|
|
There was a problem hiding this comment.
This file should not be modified. According to the task instructions and the previous review feedback, all changes must be made only in the src/scripts/main.js file. Please revert the changes made here.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on the JavaScript logic in src/scripts/main.js! You've perfectly implemented the calculations for the total and average populations, and the number formatting is spot on.
I am approving this submission as an educational override, but please note that the task required that only the main.js file be changed. Your submission included modifications to src/styles/main.scss, which should have been reverted. In future projects, be sure to adhere strictly to the file modification constraints.
Overall, your JavaScript skills are looking great. Keep up the fantastic work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| .total, | ||
| .average { | ||
| font-size: 20px; | ||
|
|
There was a problem hiding this comment.
Per the task instructions and the previous review feedback, this file should not be changed. Please revert any modifications made to src/styles/main.scss.
2 participants
No description provided.