Skip to content

add solution#1820

Open
ybriukhovetska wants to merge 3 commits intomate-academy:masterfrom
ybriukhovetska:develop
Open

add solution#1820
ybriukhovetska wants to merge 3 commits intomate-academy:masterfrom
ybriukhovetska:develop

Conversation

@ybriukhovetska
Copy link

Copy link

@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 am requesting changes because there are a few critical issues that can break functionality and violate the task requirements. First, your capitalize helper in src/scripts/main.js assumes value is always non-empty and does value[0] directly; if an input has an empty or missing name, this can throw a runtime error, so you need a guard like if (!value) return ''; before accessing characters. Second, you currently process all input elements in each form, which will also create labels and placeholders for controls like type="submit", button, or hidden; you should explicitly skip non-text inputs so only text-like fields get labels and placeholders. Third, you assign input.id = input.name; without checking that input.name exists or is valid, which can lead to empty/duplicate/invalid IDs and break the label’s htmlFor link; add a check and either sanitize input.name or generate a safe unique id when needed. These three points directly impact correctness and checklist items around capitalization, input filtering, and proper label–input association, so they need to be fixed before the solution can be considered complete. Once you add these guards and type checks, your overall approach (creating labels with class field-label, using name for text/placeholder, and appending labels to the input’s parent) will be in good shape; optionally, you can also insert labels before the inputs for better visual order. Keep going—your core logic is solid, it just needs these robustness tweaks to be fully reliable.


✨ 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 on lines 5 to 8
function capitalize(value) {
const word = value[0].toUpperCase() + value.slice(1);

return word;

Choose a reason for hiding this comment

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

capitalize assumes value is non-empty and uses value[0] directly. If input.name is empty or undefined this will throw. Add a guard such as if (!value) return ''; or ensure you handle empty strings before accessing [0]. This addresses checklist item #3 (capitalize based on input name).

Comment on lines 15 to 16
if (!input.id) {
input.id = input.name;

Choose a reason for hiding this comment

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

Assigning input.id = input.name; without checking input.name can create invalid/empty ids. Ensure input.name exists and is a valid id (or generate a safe unique id) before using it for id and for label.for. This is important because later you set label.htmlFor = input.id (checklist item #13).

label.htmlFor = input.id;
label.textContent = capitalize(input.name);
input.placeholder = capitalize(input.name);
input.parentNode.appendChild(label);

Choose a reason for hiding this comment

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

You append the label with input.parentNode.appendChild(label), which places the label after the input. The task requires appending the label to the input parent (checklist item #15), but if the expected visual order is label before the input consider using input.parentNode.insertBefore(label, input) so the label appears before the input while still not wrapping it (checklist item #16).

Copy link

@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 am requesting changes because a couple of core requirements are not fully met yet, even though your main logic is clear and well-structured. Your script only creates labels and placeholders for type="text" and type="password" inputs (see around the conditional that checks input.type), but the task requires handling all relevant form inputs, excluding only non-data controls like submit/reset/hidden/buttons; as a result, fields such as email or tel will be missing labels and placeholders. You also derive the id directly from input.name without sanitizing or ensuring uniqueness, which can break the label–input association when names contain spaces/special characters or when multiple inputs share the same name (for example, radio groups). To fix this, broaden the set of input types you process (or explicitly skip only non-textual controls), and add a small helper to generate a safe, unique id (e.g., replace non-alphanumeric characters and add a numeric suffix if an element with that id already exists) before assigning label.htmlFor. Additionally, the task requires modifying only src/scripts/main.js, so please revert any changes made to README.md to comply with that constraint. Once these issues are addressed, your existing capitalization helper and DOM insertion logic will fit the requirements nicely, and the solution should be ready to pass.


✨ 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

Copy link

@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 am approving your solution as an educational override so you can keep progressing while you refine a few remaining details.

You did a solid job iterating over form inputs, creating label elements with the correct class, and using input.name (properly capitalized) for both labels and placeholders, which matches the core requirement of relying on the name attribute. There are, however, two important issues to address next: first, your exclusion list in src/scripts/main.js currently skips checkbox and radio inputs, but the requirement is to only skip non-data controls like submit, button, reset, hidden, image, and file, so you should remove checkbox and radio from that list so they also get labels and placeholders. Second, you’re setting label.htmlFor = input.id without guaranteeing that input.id exists; you’ll want to add a helper that generates a safe, unique id from input.name (sanitize non-alphanumeric characters and, if needed, append a numeric suffix), assign it to input.id, and then set label.htmlFor to that new id before inserting the label into the DOM.
Additionally, make sure you only set a placeholder where the placeholder attribute is supported, and consider normalizing or trimming the name value before capitalization for cleaner label text. Overall, your structure and intent are strong—these refinements will bring your solution fully in line with the requirements and improve accessibility and robustness. Keep up the good work; you’re very close to a polished, production-ready implementation.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment on lines +21 to +28
'submit',
'button',
'reset',
'hidden',
'image',
'file',
'checkbox',
'radio',

Choose a reason for hiding this comment

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

The exclusion list includes checkbox and radio, which will skip those data inputs. The task requires adding labels/placeholders relying on name for form inputs; only non-data controls (submit/button/reset/hidden/image/file) should be skipped. Remove checkbox and radio from this list so they receive labels/placeholders as well.


label.classList.add('field-label');

label.htmlFor = input.id;

Choose a reason for hiding this comment

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

You set label.htmlFor = input.id, but the script never guarantees input.id exists. If an input lacks an id the label won't be associated with the control. Add a helper to generate a safe id (e.g. from input.name by removing/replacing non-alphanumeric characters and ensuring uniqueness by appending a numeric suffix if needed), assign it to input.id, then set label.htmlFor to that id before inserting the label.

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.

2 participants