Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on the logic for calculating and formatting the population numbers! The core calculations are spot on. However, changes are needed regarding how the results are displayed on the page. The task requires updating only the text inside the <span> elements, but your code uses replaceWith(), which removes the <span> elements themselves. This alters the HTML layout, which is against the requirements. Instead of replacing the entire element, please update its content using a property like element.innerText or element.textContent.
✨ 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
| resultAverage.unshift(newAveragePopulationResul.splice(-3).join('')); | ||
| } | ||
|
|
||
| totalPopulation.replaceWith(resultTotal.join(',')); |
There was a problem hiding this comment.
The task requires you to replace the text inside the span element, not the element itself. Using replaceWith() removes the <span> from the document, which alters the HTML layout and can break styles. You should modify the content of the element instead, for example, by setting its innerText property.
src/scripts/main.js
Outdated
| } | ||
|
|
||
| totalPopulation.replaceWith(resultTotal.join(',')); | ||
| averagePopulation.replaceWith(resultAverage.join(',')); |
There was a problem hiding this comment.
Similar to the total population element, you are replacing the entire span here. The requirement is to only update the text content. Please modify the innerText of the averagePopulation element instead of replacing the whole node.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! Your solution correctly calculates and displays the total and average population, meeting all the core requirements. I am happy to approve your submission.
For future improvements, here are a few suggestions to make your code even cleaner:
- Avoid duplicate DOM queries: The
document.querySelectorAll('.population')call is made twice. You can store the result from the first call and reuse it later to improve efficiency. - Reduce code repetition: The logic for formatting numbers with thousands separators is repeated. Consider creating a single reusable function for this, or you could use the built-in
number.toLocaleString()method for a more concise solution. - There's a small typo in the
newAvaragePopulationResultvariable name.
Great job addressing the previous feedback and getting the functionality working perfectly!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| const population = document.querySelectorAll('.population'); | ||
| const averagePopulation = document.querySelector('.average-population'); | ||
| const totalPopulation = document.querySelector('.total-population'); | ||
| const newPopulation = [...document.querySelectorAll('.population')]; |
There was a problem hiding this comment.
This line queries the DOM for the same elements as on line 3. It's more efficient to query the DOM only once and reuse the resulting NodeList. The population variable from line 3 can be used directly in the for...of loop on line 9.
| totalPopulationResult += Number(onePopulation.innerText.split(',').join('')); | ||
| } | ||
|
|
||
| const newTotalPopulationResult = String(totalPopulationResult).split(''); |
There was a problem hiding this comment.
This logic for adding thousands separators is effective, but it's repeated again on lines 23-28. To avoid code duplication, consider creating a reusable function for this task. You could also look into the standard toLocaleString() method, which can format numbers like this in a single line.
| const averagePopulationResult = Math.round( | ||
| totalPopulationResult / population.length, | ||
| ); | ||
| const newAveragePopulationResul = String(averagePopulationResult).split(''); |
There was a problem hiding this comment.
There's a small typo in this variable name. It should probably be newAveragePopulationResult to be consistent.
2 participants
No description provided.