Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Good work on implementing the core logic to calculate the total and average population! However, there are a couple of critical issues that need to be addressed before this can be approved.
First, the task instructions require that only the main.js file be modified. Your submission includes changes to src/styles/_fonts.scss and src/styles/main.scss. Please revert the changes in these files.
Second, the average population value is not being formatted correctly with a thousands separator. In src/scripts/main.js, you are calling .toFixed() before .toLocaleString(). Since .toFixed() returns a string, .toLocaleString() cannot apply the correct formatting. You should call .toLocaleString() on the number directly and use its options to control the decimal places. For example:
const average = total / populations.length;
const formattedAverage = average.toLocaleString('en-US', { minimumFractionDigits: 2, maximumFractionDigits: 2 });Once you've addressed these points, your solution will be in great shape. Keep up the good effort!
✨ 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 total = numb.reduce((n1, n2) => n1 + n2, 0); | ||
|
|
||
| const average = (total / numb.length).toFixed(2); |
There was a problem hiding this comment.
The .toFixed() method converts the result of your calculation into a string. Because average is a string, calling .toLocaleString() on it later (on line 16) won't add the thousands separators as required. It's best practice to keep values as numbers for as long as possible and only format them into strings for the final display.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on correcting the number formatting for the average population! The logic in main.js is now working perfectly and meets all the functional requirements.
However, I am requesting changes because the submission still includes modifications to style files, which is against a core requirement of the task. Please revert all changes in the following files to their original state:
src/styles/_fonts.scsssrc/styles/main.scss
Once those files are reverted so that only main.js is modified, 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
| @@ -1,15 +1,19 @@ | |||
| @import 'fonts'; | |||
There was a problem hiding this comment.
As per the task instructions, you should only make changes to the main.js file. Please revert any modifications made to this SCSS file.
| 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.
As mentioned in the previous review, this file should not be modified. The font-family declaration on line 2 is incorrect for a @font-face rule; it should define a single font name, not a list of fallbacks. Please revert this file to its original state.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this solution as an educational override to help you move forward, but there are a couple of important points to review for future tasks. Great job reverting the changes in one of the style files and getting the core calculation logic correct!
Your solution is very close to perfect. The main issue is that the average population is displayed with decimals. To match the original data's style, this number should be a whole number. Consider using Math.round() on your calculated average before displaying it. Also, please remember to revert the changes in src/styles/main.scss, as the task requires that only main.js be modified.
Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| average.toLocaleString('en-US', { | ||
| minimumFractionDigits: 2, | ||
| maximumFractionDigits: 2, | ||
| }); |
There was a problem hiding this comment.
The task requires the calculated numbers to have the same numeric style as the original population data. The population numbers are integers, so the average should also be displayed as a rounded integer without decimal places. Consider using Math.round() on the average before formatting it.
|
|
||
| body { | ||
| background: #eee; | ||
| counter-reset: section; |
There was a problem hiding this comment.
This file should not be modified. As per the task instructions, all style files must remain in their original state. Please revert any changes you have made here.
2 participants
No description provided.