Skip to content

add task solution#2297

Open
pparys93 wants to merge 3 commits into
mate-academy:masterfrom
pparys93:develop
Open

add task solution#2297
pparys93 wants to merge 3 commits into
mate-academy:masterfrom
pparys93:develop

Conversation

@pparys93

@pparys93 pparys93 commented Jul 6, 2026

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because one explicit checklist requirement is still not met, even though your JavaScript implementation itself looks correct and complete. Your src/scripts/main.js correctly gathers all .population spans, converts the text values to numbers, computes total and average, and updates .average-population and .total-population with properly formatted values using toLocaleString, so the core functionality is solid and meets the JS-related requirements. However, the description/demo link still uses the placeholder <your_account> instead of your actual GitHub username, which violates [CHECKLIST ITEM #13] and is considered a blocking requirement for task completion. To fix this, update the demo link in the description file so that the GitHub URL contains your real username (for example, https://github.com/your-username/...) and commit that change. As a minor improvement (not blocking), you could also add a simple validity check around your number parsing (e.g., Number.isNaN(parsedValue)) to handle unexpected text more robustly, and run npm run lint to ensure there are no style issues. Once the demo link is updated to your actual account, your solution should fully satisfy all requirements—nice work on the main logic.


✨ 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. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because one core checklist requirement is still not met, even though your JavaScript logic in src/scripts/main.js is correct and complete. Your code correctly reads all .population spans, converts the values to numbers (handling thousands separators), filters out invalid entries, and calculates both the total and average, then renders them with toLocaleString() into .average-population and .total-population. The blocking issue is that the demo link in description.source.md still uses the placeholder https://<your_account>.github.io/js_get_data_DOM/ instead of your actual GitHub username, which violates [CHECKLIST ITEM #13] about updating the demo link. To fix this, replace <your_account> with your real GitHub username in that URL so the demo link points to your live GitHub Pages deployment. As a minor improvement, you could use parseInt(str, 10) or Number(str) explicitly when converting strings to numbers, but that is not blocking. Once you update the description file’s demo link, your solution will fully meet the task requirements and can be accepted.


✨ 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. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/scripts/main.js
'use strict';

// write your code here
const populations = document.querySelectorAll('.population');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main JS logic meets the functional requirements (parsing populations, converting to numbers, computing total/average, and formatting with thousands separators). The remaining blocking issue is outside this file: the description/demo link still uses <your_account> instead of your actual GitHub username, which violates the checklist item that requires updating the demo link. Make sure to fix the description file accordingly.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution as an educational override so you can keep progressing while learning. Your src/scripts/main.js implementation is solid: you correctly read all .population spans, safely convert their text to numbers (handling commas), filter out invalid values, and compute both total and average before writing them back to .average-population and .total-population with toLocaleString() to preserve thousands separators. This matches the main functional requirements and shows good attention to parsing and formatting details.

There is one remaining checklist-related issue outside of main.js: in description.source.md, the demo link still uses the <your_account> placeholder instead of your actual GitHub username, so the link doesn’t yet point to your live GitHub Pages deployment [CHECKLIST ITEM #X]. To fix this, update the URL in description.source.md to something like https://<your_github_username>.github.io/js_get_data_DOM/, then re-commit and push. Overall, your JavaScript work here is clean, focused, and meets the task’s logic requirements—nice job, and keep building on this kind of careful, requirement-driven implementation.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants