Skip to content

add solution#1469

Open
Yevhen-Srdk wants to merge 3 commits intomate-academy:masterfrom
Yevhen-Srdk:develop
Open

add solution#1469
Yevhen-Srdk wants to merge 3 commits intomate-academy:masterfrom
Yevhen-Srdk:develop

Conversation

@Yevhen-Srdk
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

I am requesting changes because one core checklist requirement is not yet met and this affects the intended behavior of the exercise. Both promises are required to have success and error handlers, but currently promise1 has only a success handler (then(successMessage)) without a corresponding .catch(rejectMessage), and promise2 has only an error handler (.catch(rejectMessage)) without a corresponding success handler (then(successMessage)). This means the task requirement that each promise demonstrates both paths is not fulfilled and could cause tests or automated checks to fail.

Critical issues to fix:

  • For promise1 in src/scripts/main.js, add an error handler, for example by chaining .catch(rejectMessage) after the existing then(successMessage).
  • For promise2, add a success handler, e.g. promise2.then(successMessage) before or in addition to the existing .catch(rejectMessage), so it also demonstrates the resolved case as required.

Minor but recommended improvement: before attaching the click listener, guard const logo = document.querySelector('.logo') with if (logo) { ... } (or handle the missing-element case) to avoid a possible runtime error if .logo is not present in the DOM. Once you add the missing handlers and optionally the guard, your solution will fully align with the checklist and the behavior the task is trying to teach—nice work so far on setting up the promises and DOM message functions.


✨ 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


// write your code here
const promise1 = new Promise((resolve) => {
const logo = document.querySelector('.logo');

Choose a reason for hiding this comment

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

document.querySelector('.logo') can return null and calling addEventListener on null will throw. Add a guard such as if (logo) { logo.addEventListener(...); } or handle the missing element to avoid runtime errors.

document.body.appendChild(div);
}

promise1.then(successMessage);

Choose a reason for hiding this comment

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

Missing error handler for promise1. The checklist requires error handlers for both promises — add .catch(rejectMessage) or use .then(successMessage, rejectMessage).

}

promise1.then(successMessage);
promise2.catch(rejectMessage);

Choose a reason for hiding this comment

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

Missing success handler for promise2. The checklist requires success handlers for both promises — add promise2.then(successMessage) in addition to the existing .catch(rejectMessage).

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 there is a runtime-safety issue that can throw an error and break the functionality when the .logo element is not present in the DOM. In src/scripts/main.js, you do const logo = document.querySelector('.logo'); and then immediately call logo.addEventListener('click', ...) without checking if logo is non-null, which can cause a crash instead of allowing the promises and handlers to work as expected. To fix this, wrap the event listener registration in a guard:

const logo = document.querySelector('.logo');
if (logo) {
  logo.addEventListener('click', () => {
    resolve();
  });
}

After adding this outer if (logo), remove the redundant inner if (logo) check inside the click handler, since the handler will only ever run if a valid element existed when you attached the listener. Aside from this critical issue, your implementation of the promises, the 3-second rejection, and the success/error message handlers looks solid, so once you address this null-check problem your solution 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

Comment on lines +5 to +11
const logo = document.querySelector('.logo');

logo.addEventListener('click', () => {
if (logo) {
resolve();
}
});

Choose a reason for hiding this comment

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

logo may be null — calling logo.addEventListener will throw if .logo is not present. Wrap the listener registration with an existence check, for example:

const logo = document.querySelector('.logo');
if (logo) {
  logo.addEventListener('click', () => {
    resolve();
  });
}

Also remove the redundant inner if (logo) check inside the handler once you guard before attaching the listener.

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 as an educational override so you can continue progressing while you learn, even though there is still one important issue to address.

You did a solid job meeting the core requirements: you created promise1 and promise2, had promise2 reject after about 3 seconds, and correctly used .then() and .catch() to append the appropriate <div class="message"> and <div class="message error-message"> elements with the right texts. The main problem is a runtime-safety risk in src/scripts/main.js: around the line where you do const logo = document.querySelector('.logo'); and then immediately call logo.addEventListener('click', ...), this will throw an error if .logo is missing from the page. To fix this, wrap the listener in a guard like if (logo) { logo.addEventListener('click', () => { resolve(); }); } so your script doesn’t crash when the element isn’t present.
A smaller improvement to consider is limiting the click handler to fire only once (for example by removing the listener after the first click or using the { once: true } option), so you don’t create multiple success messages on repeated clicks. Overall, your promise logic and DOM updates are well-structured—tightening up this null-check and minor behavior details will make your solution more robust while keeping the good work you’ve already done.


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

Footnotes

  1. Rate AI review example

Comment on lines +5 to +9
const logo = document.querySelector('.logo');

logo.addEventListener('click', () => {
resolve();
});

Choose a reason for hiding this comment

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

Runtime-safety: add null-check before using logo. The code does const logo = document.querySelector('.logo'); and immediately calls logo.addEventListener(...), which will throw if .logo is not found. Wrap the listener registration in a guard, for example:

const logo = document.querySelector('.logo');
if (logo) {
  logo.addEventListener('click', () => {
    resolve();
  });
}

This prevents a runtime crash when the .logo element is missing and satisfies the previous HIGH-priority guidance.

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