Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job setting up promise1 and promise2 with the correct asynchronous logic! Your solution is on the right track, but it needs a couple of adjustments to meet all the requirements.
There are two main issues to address:
- Incorrect DOM Target: The success and error messages are currently appended to the
.logoelement (lines 17 and 28). The requirements state these should be appended directly to thedocument.body. - Incomplete Promise Handling: The task requires both promises to have both success (
.then()) and error (.catch()) handlers. Currently,promise1is missing its error handler, andpromise2is missing its success handler.
Please make these corrections. You're very close to a complete solution!
✨ 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
src/scripts/main.js
Outdated
| promise1.then((res) => { | ||
| logo.append(divResolve); | ||
| divResolve.textContent = res; | ||
| }); |
There was a problem hiding this comment.
This handler for promise1 has two issues:
- The success message
<div>should be appended to thedocument.body, not the.logoelement as is currently done on line 17. - The requirements state that both promises need success and error handlers. This promise is missing an error handler (
.catch()).
src/scripts/main.js
Outdated
| promise2.catch((rej) => { | ||
| logo.append(divReject); | ||
| divReject.textContent = 'Promise was rejected!'; | ||
| divReject.classList.add('error-message'); | ||
| }); |
There was a problem hiding this comment.
This handler for promise2 has two issues:
- The error message
<div>should be appended to thedocument.body, not the.logoelement as is currently done on line 28. - The requirements state that both promises need success and error handlers. This promise is missing a success handler (
.then()).
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great progress on this! You've correctly added the .then() and .catch() handlers to both promises. However, this requires changes because the task specifies that both promises must have functional success and error handlers. Currently, some of your handlers are empty.
- The
.catch()block forpromise1is empty. It needs to append the error message to the body. - The
.then()block forpromise2is empty. It should append the success message to the body.
Please implement the logic inside these empty handlers to complete the requirements. You're very close!
✨ 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
src/scripts/main.js
Outdated
| body.appendChild(divResolve); | ||
| divResolve.textContent = res; | ||
| }) | ||
| .catch(); |
There was a problem hiding this comment.
This error handler for promise1 is currently empty. The task requires both promises to have a functional error handler. It should append the error message to the body, just like you did for promise2.
src/scripts/main.js
Outdated
| divReject.textContent = 'Promise was rejected!'; | ||
| divReject.classList.add('error-message'); | ||
| }) | ||
| .then(); |
There was a problem hiding this comment.
This success handler for promise2 is empty. According to the requirements, both promises need a success handler that appends the success message to the body. Please add the implementation here, similar to promise1's .then() block.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! I'm approving your solution as an educational override so you can continue progressing.
You've correctly set up the handlers for both promises, which is great! However, there's a subtle issue with the promise chain for promise2. Because you've chained .catch() before .then(), both the success and error messages will appear when the promise is rejected. The .catch() block handles the error, but then the chain continues to the .then() block as if it were a success.
To ensure only the correct message is displayed, the chain should be promise2.then(...).catch(...), just like you correctly did for promise1.
You're very close to a perfect solution. Keep up the fantastic effort!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| promise2 | ||
| .catch((rej) => { | ||
| body.appendChild(divReject); | ||
| divReject.textContent = 'Promise was rejected!'; | ||
| divReject.classList.add('error-message'); | ||
| }) | ||
| .then((res) => { | ||
| body.appendChild(divResolve); | ||
| divResolve.textContent = 'Promise was resolved!'; | ||
| }); |
There was a problem hiding this comment.
The way you've chained .catch() before .then() here introduces a subtle bug. When promise2 rejects, the .catch() block correctly handles the error and appends the rejection message. However, because the error is now 'caught', the promise chain becomes resolved, causing the following .then() block to execute as well.
To ensure only the error handler runs on rejection, the chain should be promise2.then(...).catch(...), just like you did for promise1.
2 participants
No description provided.