Skip to content

added solution#1561

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

added solution#1561
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 some core checklist requirements and the related test are still not met, which blocks the task from being considered complete. The success handler for the resolved promise does not create the exact required element <div class="message">Promise was resolved!</div>, so Cypress cannot find .message:not(.error-message) after clicking the logo [CHECKLIST ITEM #6]. The error handler for promise2 currently only adds the error-message class and uses error.message instead of appending <div class="message error-message">Promise was rejected!</div> with the exact text [CHECKLIST ITEM #7]. Also, promise1 is defined inside the click handler instead of as a standalone promise1 variable at module scope, and it has no error handler at all, which violates the requirement to have two named promises with both success and error handlers [CHECKLIST ITEMS #1, #4, #5].

To fix this, define const promise1 = new Promise(...) at the top level (like promise2), then use the .logo click handler only to trigger its resolution or rejection rather than to recreate the promise. Add both .then (success) and .catch (error) handlers for each promise; in success handlers, append a <div class="message"> whose textContent is exactly Promise was resolved!, and in error handlers append <div class="message error-message"> with text exactly Promise was rejected!. Once these changes are made, your promises will match the naming, handler, and DOM requirements and the failing Cypress test should pass. You already have the basic asynchronous logic in place, so adjusting the structure and DOM details will bring your solution in line with the specification.


✨ 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
});

logo.addEventListener('click', () => {
const promise1 = new Promise((resolve, reject) => {

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 two promises named promise1 and promise2 as standalone variables. Here promise1 is created inside the click handler, so you don�t have a dedicated promise1 variable at module scope as required by checklist item #1.

Comment thread src/scripts/main.js Outdated
Comment on lines +12 to +15
promise2.catch((error) => {
const division = document.createElement('div');

division.classList.add('error-message');

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 error handler for promise2 must append a <div class="message error-message"> with the text Promise was rejected! to the body. Here you only add error-message class and use error.message instead of the exact required text, so this does not satisfy checklist items #7 and #8.

Comment thread src/scripts/main.js Outdated
Comment on lines +25 to +28
promise1.then((result) => {
const division = document.createElement('div');

division.textContent = result;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Success handlers must append a <div class="message"> with text Promise was resolved!. This handler sets the text correctly but does not assign the message class, so it doesn�t meet checklist items #6 and #8.

Comment thread src/scripts/main.js Outdated
Comment on lines +25 to +30
promise1.then((result) => {
const division = document.createElement('div');

division.textContent = result;
document.body.appendChild(division);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is no error handler for promise1. The description requires both success and error handlers for each promise, so you need to add a catch (or failure callback) for promise1 that appends the specified error div.

@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 critical structural requirements missing that will block the tests from passing. The main issue is that promise1 is not defined as a top-level named promise with its own .then and .catch handlers; instead, it is created inside the .logo click handler, which violates the requirement that the click should only trigger resolution or rejection of an already-defined promise1 [CHECKLIST ITEM #1, #4, #5]. You also haven’t added an error handler (.catch) for promise1, so it currently has no error path, even though both promises are required to have success and error handlers. For promise2, the .catch handler appends the right classes but uses error.message for the text; the requirement is that the handler always appends <div class="message error-message">Promise was rejected!</div> with that exact text, independent of the error object [CHECKLIST ITEM #7]. In addition, the success handler behavior for promise1 must be implemented as a .then on the promise itself that appends <div class="message">Promise was resolved!</div> when the promise resolves, rather than building this element only inside the click logic.

To fix this, move promise1 to module scope as const promise1 = new Promise(...), attach .then and .catch to it that append the exact required <div> structures, and update the .logo click handler so it only triggers resolve or reject of that existing promise instead of recreating it. For promise2, keep the 3-second reject but change the .catch to set div.textContent = 'Promise was rejected!' and consider adding a .then success handler so both outcomes are covered. Also make sure the GitHub demo link in the description file uses your actual username. You’re close on the overall behavior; once these structural and text-precision issues are addressed, your solution should align well with the task and pass the tests.


✨ 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
Comment on lines +5 to +8
logo.addEventListener('click', async () => {
const promise1 = new Promise((resolve, reject) => {
resolve('Promise was resolved!');
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

promise1 is being created inside the click handler and used only with a success path; the requirements and checklist call for a named promise1 at module scope with both success and error handlers, and the logo click should only trigger its resolution or rejection, not recreate the promise each time.

Comment thread src/scripts/main.js Outdated
Comment on lines +5 to +17
logo.addEventListener('click', async () => {
const promise1 = new Promise((resolve, reject) => {
resolve('Promise was resolved!');
});

const result = await promise1;

const division = document.createElement('div');
division.classList.add('message');

division.textContent = result;
document.body.appendChild(division);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is no error handler (.catch) for promise1, but the task requires both success and error handlers for each of the two promises [CHECKLIST ITEMS #4, #5]. Consider adding a .catch for promise1 that appends the specified error <div> when the promise is rejected.

Comment thread src/scripts/main.js Outdated
Comment on lines +25 to +30
promise2.catch((error) => {
const division = document.createElement('div');
division.classList.add('message');

division.classList.add('error-message');
division.textContent = error.message;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In the catch handler for promise2, you’re using error.message as the text, but the requirement is to append a <div class="message error-message"> whose text content is exactly Promise was rejected!, independent of the Error instance.

@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 you refine your understanding of promises and handlers.

You did well defining promise2 at the top level with .then and .catch, and wiring it to reject after 3 seconds as required. The main structural issue is that promise1 is not a top-level named promise; instead, it’s created inside createPromise1() and handled via await in the click handler, which conflicts with the requirement for const promise1 = new Promise(...) plus its own .then and .catch attached at the top level [CHECKLIST ITEM #1, #4]. To fix this, define promise1 directly at the top level and attach a .then that appends <div class="message">Promise was resolved!</div> and a .catch that appends <div class="message error-message">Promise was rejected!</div>; the .logo click should only trigger resolving or rejecting that existing promise, not create a new one.

For promise2, your structure is good, but both the success and error handlers should always use the exact fixed strings Promise was resolved! and Promise was rejected! instead of result or error.message so they match the tests precisely [CHECKLIST ITEM #7]. As a minor improvement, remember to replace <your_account> in the demo link with your actual GitHub username so the project metadata is complete. Overall, you’re very close—your understanding of promises and DOM updates is solid, and with these structural and text-precision tweaks, your solution will fully align with the requirements.


✨ 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
Comment on lines +5 to +9
function createPromise1() {
return new Promise((resolve, reject) => {
resolve('Promise was resolved!');
});
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

promise1 is required to be a top-level named promise (const promise1 = new Promise(...)) with its own .then and .catch handlers. Defining createPromise1() that creates and resolves the promise internally means promise1 is not a named top-level promise as specified.

Comment thread src/scripts/main.js
Comment on lines +11 to +13
logo.addEventListener('click', async () => {
try {
const promise1 = await createPromise1();

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 click handler currently creates and resolves the promise via await createPromise1(). Per the requirements, the .logo click should only trigger resolution or rejection of an already-defined promise1, not create a new promise instance each time.

Comment thread src/scripts/main.js
Comment on lines +15 to +19
const division = document.createElement('div');

division.classList.add('message');
division.textContent = promise1;
document.body.appendChild(division);

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 success path for promise1 is implemented inside the click handler instead of as a .then attached directly to a top-level promise1. The checklist requires a .then on promise1 that appends <div class="message">Promise was resolved!</div> when the promise resolves.

Comment thread src/scripts/main.js
Comment on lines +20 to +27
} catch (error) {
const division = document.createElement('div');

division.classList.add('message',
'error-message');
division.textContent = error.message;
document.body.appendChild(division);
}

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 error handling for promise1 is implemented via try/catch around the async function rather than a .catch handler attached to promise1 itself. Both promises must have explicit success and error handlers using .then and .catch.

Comment thread src/scripts/main.js
Comment on lines +44 to +48
promise2.catch((error) => {
const division = document.createElement('div');

division.classList.add('message', 'error-message');
division.textContent = error.message;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In the promise2 rejection handler, the text content must be exactly Promise was rejected! per the description, not error.message. Use the fixed string regardless of the error object.

Comment thread src/scripts/main.js
Comment on lines +36 to +41
promise2.then((result) => {
const division = document.createElement('div');

division.classList.add('message');
division.textContent = result;
document.body.appendChild(division);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In the promise2 success handler, the description requires the success handler to append a <div class="message"> with text exactly Promise was resolved!. Using result here will not satisfy the requirement if the promise is ever resolved.

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