Skip to content

Added solution#1903

Open
Skoulskiy wants to merge 3 commits into
mate-academy:masterfrom
Skoulskiy:develop
Open

Added solution#1903
Skoulskiy wants to merge 3 commits into
mate-academy:masterfrom
Skoulskiy:develop

Conversation

@Skoulskiy

Copy link
Copy Markdown

No description provided.

@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 there are still a few core requirements that are not met, and they are likely to cause test failures.

Critical issues that must be fixed

  1. In src/scripts/main.js, your selector document.querySelectorAll('input') (near the top of the main loop) pulls in all inputs on the page, but the instructions explicitly require selecting inputs from the form tag only. You should first get the form, e.g. const form = document.querySelector('form');, and then select inputs from that form with form.querySelectorAll('input') so you only affect the targeted fields.
  2. For placeholder text, you currently use input.name.toUpperCase(), which uppercases the entire string. The requirement is to “capitalize” based on the input name, which typically means only the first character should be uppercase (e.g. emailEmail). Update this transformation to capitalize just the first letter, such as using const placeholder = name[0].toUpperCase() + name.slice(1);, and then assigning that to input.placeholder.
  3. When inserting labels, you are calling input.before(label), which places the label before the input but does not explicitly append it to the parent container as required. The task specifies that labels must be appended to the parent element of the input, so use input.parentElement.appendChild(label) (or an equivalent approach via input.parentElement) to ensure the DOM structure matches the specification.

Minor notes

  • Aside from these points, your work on creating labels, setting the for attribute, adding the field-label class, and using a single loop in main.js is solid and aligns well with the task. Once you adjust the form scoping, capitalization logic, and label appending, your solution should fully satisfy the requirements. Keep going—these are straightforward fixes and you’re very close to a complete, correct implementation.

✨ 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 inputs = 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.

This selects all inputs in the document, but the instructions require getting inputs from the form tag specifically. Consider selecting the form first and then querying its inputs.

Comment thread src/scripts/main.js Outdated
label.setAttribute('for', input.id);
label.textContent = input.name || 'Field label';

input.placeholder = input.name.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.

Using input.name.toUpperCase() uppercases the entire string (e.g. emailEMAIL), while the requirement is to "capitalize" based on the input name (typically meaning first letter uppercase). Adjust the transformation to only capitalize the first character.

Comment thread src/scripts/main.js Outdated

input.placeholder = input.name.toUpperCase();

input.before(label);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Inserting the label with input.before(label) does not follow the explicit requirement to append labels to the parent container of the input. Use the input's parent element to append the label instead.

@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 there is still a critical mismatch with the explicit checklist requirements that could cause automated tests to fail. In src/scripts/main.js, you currently insert labels with input.parentElement.insertBefore(label, input), but the checklist items [CHECKLIST ITEM #16] and [CHECKLIST ITEM #21] explicitly require using input.parentElement.appendChild(label). Even though both attach the label to the same parent, tests may assert that appendChild is used, so this is considered a blocking requirement rather than a stylistic preference.

To fix this, locate the line where you call insertBefore (around the label insertion logic in your main loop) and replace it with input.parentElement.appendChild(label), ensuring that you still preserve the expected visual order if that matters for the UI. Additionally, update the demo link in your project description file to replace the <your_account> placeholder with your actual GitHub username to satisfy [CHECKLIST ITEM #9]. As a minor improvement, you could clean up the unreachable fallback label.textContent = input.name || 'Field label'; since you already guard with if (!input.name) return;, but this is not blocking.

Once you switch to appendChild and update the demo link, your implementation will align fully with the functional and structural requirements, as the rest of your label and placeholder logic is already well-implemented.


✨ 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

input.placeholder = capitalized;
}
input.parentElement.insertBefore(label, 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 checklist (items #16 and #21) requires appending labels to the parent container via input.parentElement.appendChild(label). Using insertBefore(label, input) still attaches the label to the same parent but does not follow the specified method and may not match tests that expect appendChild. Consider switching to appendChild on input.parentElement here.

@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 as an educational override so you can keep progressing while learning. You’ve done a strong job in main.js: you correctly iterate over all form elements, find their inputs, create label.field-label elements, set for to the input id, and derive both the label text and placeholder from input.name with proper capitalization, all within a single loop as required. This matches the core requirements and respects the constraint of modifying only main.js.

There are two small improvements to keep in mind. First, because you already return early when !input.name, the fallback like label.textContent = input.name || 'Field label' is never used; you can safely simplify this to label.textContent = input.name to keep the code cleaner. Second, remember to update the demo link in the description file by replacing <your_account> with your actual GitHub username so that [CHECKLIST ITEM] about the demo link is satisfied. Overall, your implementation shows good attention to the task’s DOM and naming requirements—nice work, and keep building on this solid foundation.


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

Footnotes

  1. Rate AI review example

Comment thread src/scripts/main.js
label.classList.add('field-label');

label.setAttribute('for', input.id);
label.textContent = input.name || 'Field label';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Because you already return early when !input.name, the fallback || 'Field label' will never be used; you can simplify this to label.textContent = input.name if you want to keep the code minimal.

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