Skip to content

add task solution#1815

Open
naviailpach wants to merge 2 commits intomate-academy:masterfrom
naviailpach:develop
Open

add task solution#1815
naviailpach wants to merge 2 commits intomate-academy:masterfrom
naviailpach:develop

Conversation

@naviailpach
Copy link

No description provided.

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 few checklist-related requirements are not yet met and will likely cause tests to fail. First, your selector currently uses document.querySelectorAll('input'), which grabs every input on the page; you must only process inputs inside the form element (e.g., query the form first and then form.querySelectorAll('input')) [CHECKLIST ITEM #16]. Second, you assign input.id = ${input.name}-input`` before checking that input.name exists; this can produce ids like `undefined-input`, so move the `if (!input.name) return;` check above any use of `input.name` or guard the id/label creation accordingly [CHECKLIST ITEM #19]. Third, you append labels after inputs with `parentNode.appendChild(label)`, but the expected DOM/visual order is usually label before input; use `input.parentNode.insertBefore(label, input)` so labels appear in the correct position [CHECKLIST ITEM #20]. Finally, don’t forget to update the demo link and replace `<your_account>` with your actual GitHub username in the task description/README as required. You’ve done a solid job with generating labels, placeholders, and capitalization; fix these issues, re-run the tests, and you should be in good shape.


✨ 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

'use strict';

// write code here
const inputs = document.querySelectorAll('input');

Choose a reason for hiding this comment

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

Selector currently grabs every input in the document. Checklist item #16 requires getting inputs from the form tag only. Use a form-scoped selector like document.querySelectorAll('form input') or query the form first and then form.querySelectorAll('input').

Comment on lines 6 to 7
if (!input.id) {
input.id = `${input.name}-input`;

Choose a reason for hiding this comment

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

You assign input.id = ${input.name}-input`` before verifying input.name exists. If an input lacks a `name`, this creates an `undefined-input` id. Move the `if (!input.name) return;` check above this assignment or guard the id assignment (relates to checklist item #19).

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

Choose a reason for hiding this comment

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

Using parentNode.appendChild(label) appends the label after the input. The task requires appending the label to the input's parent container but the visual/expected order usually places the label before the input. Consider using input.parentNode.insertBefore(label, input) to insert the label before its input so the DOM order matches the expected layout (checklist item #20).

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 because it meets all the functional requirements: you correctly process only form inputs, rely on name to generate labels and placeholders, add the field-label class, set htmlFor to the input’s id, and insert the label before the input without wrapping it. The only issue I see is minor: in src/scripts/main.js, you assign input.id = ${input.name}-input`` before checking the type, so submit, `button`, and `hidden` inputs may get an id even though you immediately return and don’t create labels or placeholders for them. This doesn’t break any requirements or functionality, but for cleaner DOM changes you could either move the type guard above the id assignment or only set the id after confirming the input type is one you want to process. Everything else in your implementation aligns well with the checklist expectations, including capitalization of placeholders and the structure of inserted labels. Nicely done overall; the code is clear, focused, and follows the constraints given in the task.


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

Footnotes

  1. Rate AI review example

}

if (!input.id) {
input.id = `${input.name}-input`;

Choose a reason for hiding this comment

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

You assign input.id = ${input.name}-input`` before checking the input type. Because you return for submit, `button`, and `hidden` inputs later (lines 14–18), those inputs may receive an id even though no label/placeholder is created. Consider moving the type check (lines 14–18) above the id assignment or only setting `id` after confirming the input will be processed.

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