Skip to content

Develop#1758

Open
PAVLO111 wants to merge 10 commits into
mate-academy:masterfrom
PAVLO111:develop
Open

Develop#1758
PAVLO111 wants to merge 10 commits into
mate-academy:masterfrom
PAVLO111:develop

Conversation

@PAVLO111
Copy link
Copy Markdown

@PAVLO111 PAVLO111 commented May 7, 2026

Copy link
Copy Markdown

@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

Your implementation creates the correct nested ul/li structure and uses recursion properly, but it fundamentally ignores the element parameter as required by the task. Instead of appending to the passed DOM element, your code appends everything to document.body, and recursive calls pass null instead of the parent element. This causes a Cypress test to fail because the #tree container element is never properly created or used. The fix is straightforward: append the created ul to the element parameter instead of document.body, and pass the parent li (or a nested ul) as the element in recursive calls so children are properly nested within their parent containers.


✨ 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 Outdated
Comment on lines +31 to +32
myTree.append(firstList);
document.body.append(myTree);
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 function ignores the element parameter and appends myTree to document.body instead. The task requires using the element parameter to build the tree within the passed DOM element.

Comment thread src/scripts/main.js Outdated

const testLength = Object.keys(data[key]).length;

if (testLength !== 0) {
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 recursive call passes null instead of properly using the element parameter. The function should be designed so recursive calls work with their respective parent elements.

Comment thread src/scripts/main.js

return myTree;
}

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 final tree is never appended to the tree element. Consider structuring the function so the initial call properly builds and appends to the element parameter.

Copy link
Copy Markdown

@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

Your code has two critical issues that prevent it from working correctly:

  1. Line 24 reassigns the element parameter — This violates ESLint's no-param-reassign rule and blocks the build. You need to create a separate const ul = document.createElement('ul') variable instead of overwriting element.

  2. Lines 31-32 append to document.body instead of the container — The ul should be appended to the passed element parameter (the #tree container), not document.body. Also, append the ul once after the loop finishes, not inside it.

The function should create a const ul = document.createElement('ul'), use element as the target container to append that ul to, and then build the list items inside the ul.


✨ 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 Outdated

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

Choose a reason for hiding this comment

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

Line 24 overwrites the element parameter. The parameter should be used as the container, not replaced. Remove this line - you should create the ul element but append it to the passed element parameter instead.

Comment thread src/scripts/main.js Outdated
Comment on lines +31 to +32
element.append(firstList);
document.body.append(element);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Line 31-32 appends the created ul to document.body instead of the element parameter. The ul should be appended to the passed element (container), not document.body. Also, the ul should only be appended once, outside the loop, after all li elements are added.

Copy link
Copy Markdown

@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'm approving this submission to help you progress while learning. The core logic for the recursive tree creation is in place, which is a great start! However, there are critical issues that are causing the Cypress tests to fail. 1. Line 21: Change document.body.append(firstList) to element.append(firstList) — your function receives an element parameter but isn't using it to append the tree. 2. Line 32: Change createTree(null, data[key]) to createTree(ul, data[key]) — the recursive call needs to pass the newly created ul so nested items append to the correct parent. 3. Missing ul for container: Create a ul element, append it to element, then append li items to that ul — the commented code at the bottom of your file shows this pattern correctly. The good news: your understanding of recursion and object key iteration is solid! Once you wire up the element parameter and fix the recursive call, the tree structure should render correctly. Keep going — you're on the right track!


✨ 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 Outdated
Comment on lines +29 to +30
// element.append(firstList);
document.body.append(firstList);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Items are appended to document.body instead of the element parameter. The container should receive the tree structure, not document.body. Uncomment element.append(firstList) and remove line 30.

Comment thread src/scripts/main.js Outdated

firstList.append(ul);

createTree(null, data[key]);
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 recursive call passes null instead of the ul element. When there are nested objects, you need to pass the newly created ul so the recursive call appends nested items to it. Change createTree(null, data[key]) to createTree(ul, data[key]).

Comment thread src/scripts/main.js Outdated
Comment on lines +23 to +44
function createTree(element, data) {
// WRITE YOUR CODE HERE
for (const key in data) {
const firstList = document.createElement('li');

firstList.textContent = key;

// element.append(firstList);
document.body.append(firstList);

const testLength = Object.keys(data[key]).length;

// console.log(testLength);

if (testLength > 0) {
const ul = document.createElement('ul');

firstList.append(ul);

createTree(null, data[key]);
}
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Missing ul creation for the container. According to the requirements, you should create a ul element, append it to element, and then append li items to that ul. The commented code at the bottom shows this pattern correctly.

@PAVLO111
Copy link
Copy Markdown
Author

PAVLO111 commented May 8, 2026

It seems you’ve reviewed a previous version of the code. This version is different, as I’ve implemented significant changes based on your earlier feedback. I’m currently trying to figure out how to attach a screenshot to the comments to show you the current results, so please bear with me while I sort out these technical issues.
Знімок екрана (1402)

Copy link
Copy Markdown

@2pasha 2pasha left a comment

Choose a reason for hiding this comment

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

lgtm 🔥

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.

3 participants