impl#1550
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 several explicit task requirements that tests will depend on are not yet met, so the current solution is functionally incomplete. The task requires two promises named promise1 and promise2, but in src/scripts/main.js they are currently named firstPromise and secondPromise, which will cause identifier-based tests to fail. Each of these promises must have both a success (.then) and an error (.catch) handler attached: right now firstPromise only has a success handler and secondPromise only has an error handler, so you need to add the missing handlers to each.
The handlers also need to set the text content explicitly, not via the resolved/rejected values: in all success handlers, create/append a <div class="message"> and set div.textContent = 'Promise was resolved!', and in all error handlers create/append a <div class="message error-message"> and set div.textContent = 'Promise was rejected!'. Currently, the success handler uses the resolved value and the error handler uses err.message, which does not follow the literal-text requirement and may fail strict tests. On the positive side, you correctly resolve the first promise on .logo click and reject the second after 3 seconds, and your DOM manipulation pattern is close to what’s needed. Once you rename the promises, add the missing handlers, and set the exact required strings in the handlers, your solution should align well 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
| 'use strict'; | ||
|
|
||
| // write your code here | ||
| const firstPromise = new Promise((resolve) => { |
There was a problem hiding this comment.
The task requires the first promise to be named promise1, and it also needs both success and error handlers attached; consider renaming and adding a .catch handler for error cases.
| }); | ||
| }); | ||
|
|
||
| const secondPromise = new Promise((resolve, reject) => { |
There was a problem hiding this comment.
The task requires promise2 as the identifier and also needs a success handler in addition to the error handler; adjust the name and add a .then handler.
| const div = document.createElement('div'); | ||
|
|
||
| div.className = 'message'; | ||
| div.textContent = value; |
There was a problem hiding this comment.
According to the requirements, the success handler itself should set div.textContent = 'Promise was resolved!' explicitly rather than relying on the resolved value.
| const div = document.createElement('div'); | ||
|
|
||
| div.className = 'message error-message'; | ||
| div.textContent = err.message; |
There was a problem hiding this comment.
For the error handler, the requirements call for div.textContent = 'Promise was rejected!' explicitly, instead of using err.message.
2 participants
No description provided.