Repository navigation
add task solution - #1973
add task solution#1973MarcelloTar wants to merge 4 commits into
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution has two critical issues that prevent it from meeting the task requirements. First, you're using document.querySelectorAll('input') which selects all inputs on the page, but the requirement specifies to get inputs from the form tag - you should use document.querySelector('form').querySelectorAll('input') instead. Second, you're setting the label's for attribute to input.name, but the requirement states it should match the input's id - use label.htmlFor = input.id instead. Both of these are core requirements that must be fixed for the task to be considered complete.
✨ 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
|
|
||
| // write code here | ||
|
|
||
| const inputs = document.querySelectorAll('input'); |
There was a problem hiding this comment.
The requirement specifies to get inputs from the form tag, but this selects all inputs on the page. Use document.querySelector('form').querySelectorAll('input') or document.querySelector('form').inputs instead.
| const attName = input.name; | ||
| const label = document.createElement('label'); | ||
|
|
||
| label.htmlFor = attName; |
There was a problem hiding this comment.
The for attribute should match the input's id, not its name. Use input.id instead of attName.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has one critical issue that must be fixed. The code currently uses document.querySelectorAll('form input') which selects inputs from ALL forms on the page. However, the requirement explicitly specifies using document.querySelector('form').querySelectorAll('input') to get inputs only from the first form element. This is a core requirement that changes the functionality - you need to scope the selection to just the first form tag, not all forms on the page. Please update the selector to use document.querySelector('form').querySelectorAll('input') instead.
✨ 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
|
|
||
| // write code here | ||
|
|
||
| const inputs = document.querySelectorAll('form input'); |
There was a problem hiding this comment.
Use document.querySelector('form').querySelectorAll('input') instead of document.querySelectorAll('form input') to get inputs only from the first form tag on the page, as required by the task.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved with an educational override to help you progress while learning. You've correctly implemented the input selection using document.querySelector('form').querySelectorAll('input') and properly created label elements with the required attributes. However, there's an important fix needed: use input.parentElement.appendChild(label) instead of insertAdjacentElement('beforebegin', label) to append labels as children of the parent container, not as siblings. Additionally, make sure the sign-in form also has the required placeholders and labels for the email and password inputs - the Cypress tests are currently failing for those fields. Keep up the good work on the sign-up form implementation!
✨ 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
| label.textContent = attName; | ||
| label.classList.add('field-label'); | ||
|
|
||
| input.insertAdjacentElement('beforebegin', label); |
There was a problem hiding this comment.
The requirement states to 'append label elements to the parent container of each input'. Using insertAdjacentElement('beforebegin', label) inserts the label as a sibling before the input, not as a child of the parent container. Use input.parentElement.appendChild(label) instead to properly append to the parent container.
DEMO LINK