add solution - #1950
add solution#1950justnicko wants to merge 3 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 implementation has two critical issues that violate explicit task requirements. First, you're using document.querySelectorAll('.field-text') to select elements, but the task explicitly requires getting all inputs from a form tag on the page [CHECKLIST ITEM 1]. You should select the form element first, then query its inputs. Second, you're using input.before(label) to insert labels as siblings before the input, but the requirement states to 'Append label elements to the parent container of input' [CHECKLIST ITEM 1]. Use input.parentNode.appendChild(label) instead to place the label as a child of the parent container. These two fixes are essential for your code to meet the task specifications and pass the automated tests.
✨ 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
| 'use strict'; | ||
|
|
||
| // write code here | ||
| const fieldInputs = document.querySelectorAll('.field-text'); |
There was a problem hiding this comment.
The code uses document.querySelectorAll('.field-text') which selects elements by class, but the task requirement #1 explicitly states to 'Get all inputs from form tag on the page'. You should select inputs from a form element, e.g., document.querySelector('form').querySelectorAll('input') or form.querySelectorAll('input').
| label.htmlFor = input.id; | ||
| input.placeholder = input.name[0].toUpperCase() + input.name.slice(1); | ||
|
|
||
| input.before(label); |
There was a problem hiding this comment.
The code uses input.before(label) which inserts the label as a sibling element before the input. However, requirement #4 states to 'Append label elements to the parent container of input'. Use input.parentNode.appendChild(label) instead to append the label as a child of the parent container.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has two critical blockers that must be fixed:
1. JavaScript - Incorrect selector approach: Your code uses document.querySelectorAll('.field') to select div wrappers, but the task explicitly requires getting all inputs from a form tag on the page (checklist item 1). You need to first select the form element using document.querySelector('form'), then get inputs directly from it using form.querySelectorAll('input').
2. HTML - Missing required IDs: All 12 Cypress tests are failing because your form inputs are missing the required id attributes. Add these IDs to your input elements: sign-up-name, sign-up-last-name, sign-up-email, sign-up-password, sign-in-email, and sign-in-password.
The good news is that your label creation logic and placeholder logic are working correctly once the inputs are selected properly. Focus on fixing the selector approach first, then ensure your HTML has the proper IDs for the tests to locate the elements.
✨ 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
| 'use strict'; | ||
|
|
||
| // write code here | ||
| const fields = document.querySelectorAll('.field'); |
There was a problem hiding this comment.
The task requires getting all inputs FROM the form tag on the page. Currently selecting .field elements instead. Change this to first select the form element (e.g., document.querySelector('form')), then get inputs from it.
| const fields = document.querySelectorAll('.field'); | ||
| let counter = 0; | ||
|
|
||
| for (const field of fields) { |
There was a problem hiding this comment.
After selecting the form, you should get inputs directly from it using form.querySelectorAll('input') or similar, rather than querying .field elements and then finding .field-text inside them.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Educational approval granted. Your code demonstrates good understanding of label creation, class assignment, text capitalization, and appending logic. However, the selector approach needs adjustment to match [CHECKLIST ITEM 1] requirements. Instead of document.querySelectorAll('.field'), start with const form = document.querySelector('form') and then const inputs = form.querySelectorAll('input') to get inputs directly from the form. Also ensure your HTML inputs have proper id attributes for the htmlFor association to work correctly. These are straightforward fixes that will bring your code fully in line with requirements.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| 'use strict'; | ||
|
|
||
| // write code here | ||
| const fields = document.querySelectorAll('.field'); |
There was a problem hiding this comment.
This selector violates checklist item 1 which requires getting all inputs from a form tag. Change to: const form = document.querySelector('form') then const inputs = form.querySelectorAll('input')
| // write code here | ||
| const fields = document.querySelectorAll('.field'); | ||
|
|
||
| for (const field of fields) { |
There was a problem hiding this comment.
The loop should iterate over input elements directly, not wrapper divs. Consider renaming to for (const input of inputs) and restructuring the logic accordingly.
| const fields = document.querySelectorAll('.field'); | ||
|
|
||
| for (const field of fields) { | ||
| const input = field.querySelector('.field-text'); |
There was a problem hiding this comment.
This line queries within a div wrapper, but should be removed since we should work with inputs directly from the form. The parent container logic on line 14 also needs adjustment.
|
|
||
| label.textContent = input.name; | ||
| label.className = 'field-label'; | ||
| label.htmlFor = input.id; |
There was a problem hiding this comment.
For htmlFor to work correctly, the input must have a proper id attribute. If inputs in HTML don't have IDs like 'sign-up-name', the label association will be broken.
| label.htmlFor = input.id; | ||
| input.placeholder = input.name[0].toUpperCase() + input.name.slice(1); | ||
|
|
||
| field.appendChild(label); |
There was a problem hiding this comment.
When iterating over inputs directly, use input.parentElement to append the label to the input's parent container instead of appending to a field div.
2 participants
No description provided.