Skip to content

add task solution#1812

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

add task solution#1812
nkoropka wants to merge 2 commits intomate-academy:masterfrom
nkoropka:develop

Conversation

@nkoropka
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

Great job on the overall logic for creating and appending the labels and placeholders! The script works correctly on the current page. However, there's one important adjustment needed to fully meet the project requirements. The task specifies getting all input elements from the form tag, but the current selector document.querySelectorAll('input') targets all inputs on the page. To ensure your code is robust and adheres to the requirements, please update the selector to be more specific, for example, by using 'form input'.


✨ 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 allInputs = document.querySelectorAll('input');

Choose a reason for hiding this comment

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

The requirements state to "Get all inputs from form tag on the page." Your current selector input will select all input elements on the entire page, regardless of whether they are in a form. To be more specific and adhere to the requirements, you should select only the inputs that are descendants of a <form> element.

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 correctly implements the required behavior in src/scripts/main.js: you select only inputs inside the form, create a <label> for each input with the field-label class, set its for attribute to the input’s id, derive the label text and placeholder from the input’s name, and append the label as a sibling within the input’s parent container. All core functional requirements for creating labels and placeholders based on the input name are met, and there are no detected runtime errors or broken features.

There are a couple of non-blocking improvements you could consider later: if an input might not have an id, you could assign one (for example from name) before setting label.htmlFor, and if you want labels to appear visually above inputs, you could use parent.insertBefore(label, input) instead of append. Also, for extra robustness, you might guard against empty name values before capitalizing the placeholder.

The CI test failure is due to an environment issue with @parcel/watcher, not a problem with your code, so it does not block this task’s acceptance. Overall, your implementation is clear, focused, and aligned with the task requirements—nice work.


✨ 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