Skip to content

js_tree-from-object-DOM#1704

Open
A1daros wants to merge 2 commits intomate-academy:masterfrom
A1daros:develop
Open

js_tree-from-object-DOM#1704
A1daros wants to merge 2 commits intomate-academy:masterfrom
A1daros:develop

Conversation

@A1daros
Copy link

@A1daros A1daros commented Feb 22, 2026

Copy link

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

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 task requirement is not yet fulfilled: the demo link placeholder has not been replaced with your actual GitHub Pages URL, which the assignment explicitly asks for and treats as part of the deliverable. Functionally, your createTree(element, data) implementation in src/scripts/main.js works: it uses recursion, DOM methods, and builds the nested <ul>/<li> structure correctly, and index.html correctly includes <div id="tree"> and the script after it. However, the missing demo URL means the task is not fully complete from the reviewer’s perspective and can block proper verification of your work.

Changes needed before this can pass:

  • Update the demo link in the project description/README to the required format, e.g. https://<your-github-username>.github.io/js_tree-from-object-DOM/, so reviewers can access a running version of your solution.

Optional, but recommended improvements (not blockers):

  • In src/scripts/main.js, add simple input validation at the top of createTree (e.g. check that element has appendChild and that data is a non-null object) to avoid cryptic runtime errors with wrong inputs.
  • Replace the for..in loop with Object.keys(data).forEach(key => { ... }) or guard with if (Object.prototype.hasOwnProperty.call(data, key)) to skip inherited properties, and only append the created <ul> if it has children (if (ul.children.length) element.appendChild(ul);).

Once you update the demo link, your solution should fully meet the task requirements; the code itself looks solid and these small robustness tweaks will make it even better.


✨ 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

/>
</head>
<body>
<div id="tree"></div>

Choose a reason for hiding this comment

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

Keep the

as the mount point for createTree. Having the <script> after this element is correct so that document.querySelector('#tree') will find it when the script runs.


function createTree(element, data) {
// WRITE YOUR CODE HERE
const ul = document.createElement('ul');

Choose a reason for hiding this comment

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

Consider validating inputs before creating DOM nodes. The function currently assumes element is a DOM element and data is an object; adding a check (e.g., ensure element has appendChild and data is an object) will produce clearer errors and enforces the requirement that element must be a DOM element and data a nested object.

// WRITE YOUR CODE HERE
const ul = document.createElement('ul');

for (const key in data) {

Choose a reason for hiding this comment

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

Using for..in iterates inherited properties too. To avoid unexpected keys, use Object.keys(data).forEach(...) or add a hasOwnProperty guard inside the loop before processing each key.

ul.appendChild(li);
}

element.appendChild(ul);

Choose a reason for hiding this comment

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

You always append the created <ul> to element, which can create empty <ul> when data is empty. Consider appending only if the <ul> has children (e.g., if (ul.children.length) element.appendChild(ul);).

Copy link

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

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 because all core functional requirements are met: createTree(element, data) correctly uses recursion to build the nested <ul>/<li> structure from the object keys, and the HTML provides the required #tree container and SCSS linkage. The main behavior—rendering the tree based on the nested food object—works as expected and satisfies the key checklist items around functionality and DOM usage.

There are a few non-blocking improvements you could make for robustness: at the top of createTree (in src/scripts/main.js), add lightweight validation to ensure element looks like a DOM node (e.g., has appendChild) and data is a non-null object, so callers get clearer errors instead of cryptic runtime failures. Also in createTree, only append the created ul to element if it actually has children, for example if (ul.children.length) element.appendChild(ul);, so you don’t end up with empty lists when data is empty. Finally, where you call createTree(tree, food), add a guard like if (tree) { ... } else { console.warn('No #tree element found'); } to avoid a TypeError if the #tree element is missing.

Overall, your implementation is clean, uses the DOM APIs properly, and demonstrates good understanding of recursion; the suggested changes are incremental hardening rather than required fixes.


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

Footnotes

  1. Rate AI review example


function createTree(element, data) {
// WRITE YOUR CODE HERE
const ul = document.createElement('ul');

Choose a reason for hiding this comment

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

Add lightweight input validation at the start of createTree. For example, verify element is a DOM node (has appendChild) and data is a non-null object. This will prevent cryptic runtime errors when callers pass unexpected values and satisfies the spirit of checklist items #5 and #6.

ul.appendChild(li);
});

element.appendChild(ul);

Choose a reason for hiding this comment

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

Only append the created ul when it actually contains list items. Consider checking if (ul.children.length) element.appendChild(ul); instead of always appending — this prevents empty

    elements from being added when data is empty.

element.appendChild(ul);
}

createTree(tree, food);

Choose a reason for hiding this comment

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

Guard the initial call to createTree(tree, food) so it fails gracefully if the #tree element is not present. e.g. if (tree) createTree(tree, food); else console.warn('No #tree element found'); — this avoids a thrown TypeError and gives a clearer failure message.

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