add solution#1563
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one core requirement is still not fully met: both promise1 and promise2 must have both success and error handlers, and right now each promise only has one type of handler. You’ve correctly implemented promise1 so it resolves on .logo click and appends <div class="message">Promise was resolved!</div> in its then handler, and promise2 correctly rejects after 3 seconds and appends <div class="message error-message">Promise was rejected!</div> in its catch handler. However, promise1 is missing an error handler (.catch) [CHECKLIST ITEM #5], and promise2 is missing a success handler (.then) [CHECKLIST ITEM #4], which means the specification is not fully satisfied.
To fix this, add a .catch to promise1 (near where you currently chain .then) that appends the same error-message div you already use for promise2, e.g. document.body.appendChild(errorDiv) with class="message error-message" and the required text. Then add a .then to promise2 (before the .catch) that appends the same success div.message you use in promise1’s success path; you can either duplicate that logic or extract a small helper like appendSuccessMessage() to keep things DRY. As a minor improvement, consider renaming any unused then/catch parameters (currently named resolve/reject in some handlers) to something like _, value, or error for clarity. Once those handlers are added, your implementation will fully cover the promise behavior the task requires—your current core logic is solid, you just need these final handlers to complete it.
✨ 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
| promise1.then((resolve) => { | ||
| const div = document.createElement('div'); | ||
|
|
||
| div.classList.add('message'); | ||
| div.textContent = 'Promise was resolved!'; | ||
|
|
||
| body.append(div); | ||
| }); |
| promise2.catch((reject) => { | ||
| const div = document.createElement('div'); | ||
|
|
||
| div.classList.add('message'); | ||
| div.classList.add('error-message'); | ||
| div.textContent = 'Promise was rejected!'; | ||
|
|
||
| body.append(div); | ||
| }); |
There was a problem hiding this comment.
promise2 currently only has an error handler via catch. To satisfy the requirement that both promises have success handlers, add a then handler here that appends the same success div.message used with promise1 when the promise resolves.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it correctly implements the two required promises, wires promise1 to resolve on .logo click, and has promise2 reject after 3 seconds, with both then and catch handlers appending the correct message elements to the body. Functionally, this matches the task specification and should work with the tests as expected.
One minor, non-blocking improvement: in src/scripts/main.js, your then/catch handlers currently use parameter names like resolve and reject even though those values are not used. Since resolve/reject usually refer to the executor functions inside new Promise(...), reusing those names for handler arguments can be confusing. You could either omit the unused parameters or rename them to something neutral like _, value, or error to make the intent clearer.
Overall, the logic is solid and fulfills the assignment requirements; any remaining changes would just be for readability and style. Nicely done on getting all the core behaviors and DOM updates in place.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| document.querySelector('.logo').addEventListener('click', resolve); | ||
| }); | ||
|
|
||
| promise1.then((resolve) => { |
There was a problem hiding this comment.
In these handlers the parameters resolve and reject are never used, and their names can be misleading (they suggest executor functions rather than handler values). Consider renaming them to something like _, value, or error, or omitting them if not needed to improve clarity.
| body.append(div); | ||
| }); | ||
|
|
||
| promise1.catch((reject) => { |
There was a problem hiding this comment.
Same note as above: the reject parameter isn’t used in this handler. Renaming or removing unused parameters will make the intent of the code clearer.
| setTimeout(reject, 3000); | ||
| }); | ||
|
|
||
| promise2.then((resolve) => { |
There was a problem hiding this comment.
Here as well, the resolve parameter in the then handler isn’t used. For cleaner code, consider using a neutral name (like _) or no parameter at all.
| body.append(div); | ||
| }); | ||
|
|
||
| promise2.catch((reject) => { |
There was a problem hiding this comment.
The reject parameter in this catch handler is unused; updating the name or removing it would avoid confusion with the executor reject in the promise constructor.
2 participants
No description provided.