add task solution#1904
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are two critical mismatches with the stated requirements that could cause the tests to fail. First, your script currently uses document.querySelectorAll('form') and iterates over all forms; the task explicitly calls for working with the single form tag on the page (e.g., document.querySelector('form')), so this violates [CHECKLIST ITEM #9] and step 1 of the description. To fix this, select just one form (for example at the top of src/scripts/main.js, something like const form = document.querySelector('form');) and then get its inputs via form.querySelectorAll('input').
Second, for password inputs you override the label text and placeholder with a hard-coded 'Password', but the requirements say to always rely on the input’s name to determine both label text and placeholder, only adjusting capitalization [CHECKLIST ITEMS #3, #4, #5, #15, #16]. Instead of branching on input.type === 'password', derive the label text and placeholder directly from input.name for all inputs and apply your capitalization/spacing logic uniformly. Aside from these two issues, your DOM manipulation (creating labels, setting for, className, and appending to the parent without wrapping the input) looks solid and aligned with the checklist. Once you scope the code to the single form and remove the password special case, your solution should fully meet the task requirements. Keep going—you’re very close to a complete and correct 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
| 'use strict'; | ||
|
|
||
| // write code here | ||
| document.querySelectorAll('form').forEach((form) => { |
There was a problem hiding this comment.
The task description and checklist require getting inputs from the form tag on the page (singular), not iterating over all forms. Consider selecting the specific form once (e.g., document.querySelector('form')) and then its inputs, matching checklist item #9 and step 1.
| if (input.type === 'password') { | ||
| displayName = 'Password'; |
There was a problem hiding this comment.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still one critical mismatch with the task requirements: the code does not always rely on the input’s name when setting label text and placeholders. In src/scripts/main.js, you branch on input.type === 'password' and override displayName with a hard-coded 'Password' string, instead of deriving it from input.name; this violates the requirement that labels and placeholders must be based on the name attribute for all inputs. This is a blocker because tests are likely checking that both the label text and placeholder are consistently sourced from input.name, so this special-casing will cause failures for password fields. To fix this, remove the if (input.type === 'password') block entirely and always compute displayName from input.name using your capitalization logic (e.g., const displayName = input.name.charAt(0).toUpperCase() + input.name.slice(1);). Once you do that, the label’s textContent and the input’s placeholder will correctly and uniformly reflect the name attribute, and your otherwise solid implementation (single form scoping, label insertion, classes, and attributes) should fully meet the task requirements. You’re very close—just align the password handling with the same name-based approach you already use for other inputs.
✨ 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
| if (input.type === 'password') { | ||
| displayName = 'Password'; | ||
| } |
There was a problem hiding this comment.
This conditional violates the requirement to always rely on the input’s name when setting label text and placeholder; avoid special-casing password inputs and derive displayName from input.name for all types instead.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you learn. Your main.js implementation is strong: you correctly select all form inputs, generate labels with the field-label class, set the for attribute from each input’s id, and derive both label text and placeholders from input.name (including proper capitalization and splitting camelCase), which aligns well with the requirements [CHECKLIST ITEM #1, #2, #3, #9, #10]. This also respects the constraint to work only in main.js and not touch the HTML/CSS structure [CHECKLIST ITEM #4].
However, test failures indicate that in src/index.html the sign-in form’s email and password fields still lack labels and placeholders that match the working sign-up form pattern, which impacts accessibility and causes the Cypress tests to fail. To improve this, mirror the structure used by the sign-up fields by ensuring each sign-in input has a surrounding .form-group, a <label for="..."> that matches the input id, and a clear placeholder (for example, placeholder="Email" / placeholder="Password") [CHECKLIST ITEM #1, #2]. Overall, your JavaScript solution is well-crafted; tightening up the HTML to match the sign-up form pattern will make your work fully consistent and test-complete, and you’re very close to a polished, production-ready result.
✨ 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
etojeDenys
left a comment
There was a problem hiding this comment.
it looks good, but you should provide a demo link and request the review again
No description provided.