feat: label and placeholders to exicting inputs#1825
feat: label and placeholders to exicting inputs#1825dmytro-halieba wants to merge 3 commits intomate-academy:masterfrom
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job setting up the DOM manipulation to add the labels and placeholders! I am requesting a couple of changes to fully meet the project requirements. Your solution is currently missing two key items:
- Missing
forattribute: Each<label>needs aforattribute that matches theidof its corresponding<input>. This is crucial for accessibility. You can set this usinglabel.htmlFor = input.id. - Uncapitalized placeholder text: The requirements specify that the placeholder text should be capitalized (e.g., 'name' should become 'Name'). You can fix this by transforming the string before setting the
placeholderattribute.
Once these are fixed, your solution will be perfect. Keep up the great work!
✨ 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
src/scripts/main.js
Outdated
| const signInInputs = Array.from(signInForm.getElementsByTagName('input')); | ||
|
|
||
| signUpInputs.forEach((input) => { | ||
| input.placeholder = input.name; |
There was a problem hiding this comment.
The task requires the placeholder text to be capitalized. For example, if input.name is 'email', the placeholder should be 'Email'.
| const label = document.createElement('label'); | ||
|
|
||
| label.textContent = input.name; | ||
| label.className = 'field-label'; |
There was a problem hiding this comment.
The label is missing its for attribute. To link the label to the input correctly, this attribute should be set to the input's id.
src/scripts/main.js
Outdated
| }); | ||
|
|
||
| signInInputs.forEach((input) => { | ||
| input.placeholder = input.name; |
There was a problem hiding this comment.
Just like with the sign-up form, the placeholder text for this input needs to be capitalized.
| const label = document.createElement('label'); | ||
|
|
||
| label.textContent = input.name; | ||
| label.className = 'field-label'; |
There was a problem hiding this comment.
This label also needs a for attribute that points to its corresponding input's id to be compliant with the requirements.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job adding the for attribute to the labels—that's a fantastic improvement for accessibility! Your solution is very close, but it requires one change to meet all the task requirements. The placeholder text for each input needs to be capitalized. You can apply the same capitalization logic you used for the label's text to fix this for both the sign-up and sign-in forms. As a suggestion for improvement, you could also refactor the duplicated logic into a single reusable function to make your code cleaner. Once the placeholder text is updated, this will be ready for approval!
✨ 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
src/scripts/main.js
Outdated
| const signInInputs = Array.from(signInForm.getElementsByTagName('input')); | ||
|
|
||
| signUpInputs.forEach((input) => { | ||
| input.placeholder = input.name; |
There was a problem hiding this comment.
According to the task requirements, the placeholder text also needs to be capitalized. You can use the same logic you used for capitalizing the label's text.
src/scripts/main.js
Outdated
| }); | ||
|
|
||
| signInInputs.forEach((input) => { | ||
| input.placeholder = input.name; |
There was a problem hiding this comment.
Just like with the sign-up form, the placeholder text here needs to be capitalized to meet the requirements.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! You've successfully addressed the previous feedback by capitalizing the placeholder text, and your script now meets all the requirements. Your solution is approved.
For future improvement, consider how you might reduce code duplication. The logic for processing the sign-up and sign-in forms is very similar. You could create a single, reusable function to handle both, which would make your code cleaner and more maintainable. This is just a suggestion for best practice, as your current solution is correct. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
DEMO LINK