Skip to content

Solution#1893

Open
Denixify wants to merge 2 commits into
mate-academy:masterfrom
Denixify:develop
Open

Solution#1893
Denixify wants to merge 2 commits into
mate-academy:masterfrom
Denixify:develop

Conversation

@Denixify

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

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 one core requirement is not yet met, which can change the behavior of the page. The task explicitly says to get all inputs from the form tag; currently in src/scripts/main.js you are using document.querySelectorAll('input'), which will also select inputs outside the form and violates that requirement. To fix this, first select the form element (e.g., const form = document.querySelector('form');) and then query its inputs with something like form.querySelectorAll('input') so only form-contained inputs are processed.

Aside from that, your handling of label creation, classes, for attributes, placeholder capitalization, and using name as the source is all correct and nicely implemented. One minor, non-blocking improvement: instead of converting the label text to full uppercase, consider reusing your capitalization logic so the label text more closely matches the original input name, which better aligns with the wording of the requirement. Once you restrict the input selection to the form element, your solution should fully meet the task’s functional requirements—good work overall, just that one critical fix is needed.


✨ 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 thread src/scripts/main.js Outdated
'use strict';

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The task requires: "Get all inputs from form tag on the page." Using document.querySelectorAll('input') will also affect inputs outside the form. Consider first selecting the form element and then querying its inputs so only form-contained inputs are processed.

Comment thread src/scripts/main.js Outdated

formInputs.forEach((input) => {
const nameValue = input.name;
const labelText = nameValue.toUpperCase();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Here you convert the input name entirely to uppercase for the label. The requirements only say to set label text based on the input name, and to capitalize the placeholder. Unless tests explicitly expect all-caps labels, you might want to reuse the capitalize logic or otherwise keep the label text closer to the original name to better match the spec.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

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 now fully matches the task requirements and resolves the earlier critical issue around input selection. You correctly restrict the query to document.querySelectorAll('form input'), ensuring only form inputs are affected as required. For each input, you create a <label> with the field-label class, set for to the input’s id, and derive the text from input.name using your capitalization helper, which aligns with the label-related requirements. You also set the placeholder text based on the input’s name with the first letter capitalized, and you prepend the label to the input’s parent container rather than wrapping or restructuring the HTML, which respects the no-layout-change constraint. All changes are confined to src/scripts/main.js, with no modifications to HTML or CSS, staying within the technical constraints of the task. Nicely done tightening up the previous issue and keeping the implementation clean and focused on the specified behavior.


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

Footnotes

  1. Rate AI review example

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