add task solution v1#1516
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the current implementation does not follow the required variable names for the two promises, which is an explicit checklist requirement and will cause the tests to fail.
Critical issues that must be fixed:
- In
src/scripts/main.js, the first promise is declared asconst firstPromise = new Promise(...), and its handlers are attached viafirstPromise.then(...).catch(...); the task requires this to be namedpromise1, so rename the declaration toconst promise1 = new Promise(...)and update all related usages (promise1.then(...).catch(...)) accordingly. - In the same file, the second promise is declared as
const secondPromise = new Promise(...)with handlers likesecondPromise.then(...).catch(...); this must be namedpromise2, so change the declaration toconst promise2 = new Promise(...)and update every reference to usepromise2instead.
Minor improvements (not blocking):
- Consider renaming the event handler parameter from
eventytoeventinsrc/scripts/main.jsto improve readability, and double-check your README demo link to ensure you’ve replaced any<your_account>placeholder with your actual GitHub username.
The promise logic and DOM notifications are otherwise implemented well, so once you align the variable names and references with the requirements, 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
| document.body.appendChild(notification); | ||
| } | ||
|
|
||
| const firstPromise = new Promise((resolve, reject) => { |
There was a problem hiding this comment.
The task requires creating a promise named promise1, but here the promise is declared as firstPromise. Rename this declaration to const promise1 = new Promise(...) to match the requirement and update all references accordingly.
| firstPromise | ||
| .then((message) => showNotification('success', message)) | ||
| .catch((err) => showNotification('error', err.message)); |
There was a problem hiding this comment.
This uses firstPromise when attaching handlers. After renaming the declaration, update this usage to promise1.then(...).catch(...) so the handlers are bound to the correctly named promise required by the task.
| .then((message) => showNotification('success', message)) | ||
| .catch((err) => showNotification('error', err.message)); | ||
|
|
||
| const secondPromise = new Promise((resolve, reject) => { |
There was a problem hiding this comment.
The task requires a promise named promise2, but this is declared as secondPromise. Rename it to const promise2 = new Promise(...) so it matches the checklist item.
| secondPromise | ||
| .then((message) => showNotification('success', message)) | ||
| .catch((err) => showNotification('error', err.message)); |
There was a problem hiding this comment.
This block attaches handlers to secondPromise. After renaming the declaration, update these calls to promise2.then(...).catch(...) so the handlers are bound to the correctly named promise required by the task.
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 (promise1 triggered by a .logo click and promise2 rejecting after 3 seconds), with appropriate .then/.catch handlers that append the specified success and error <div> notifications to document.body. The notification elements use the correct class names and messages (message for success and message error-message for errors), so the core behavior matches the task requirements and should satisfy the automated tests. As a minor readability improvement, you could rename the click handler argument from eventy to event in src/scripts/main.js so it is immediately clear to readers what the parameter represents. If your README still contains a placeholder like https://<your_account>.github.io/js_promise_basic_DOM/, consider updating it to your actual GitHub Pages URL so the demo link works as expected. Likewise, double-check that package.json includes the npm scripts mentioned in the task (for example, test and test:only) so anyone can run the checks easily. Overall, your JavaScript implementation meets the functional requirements well, with only optional polish items remaining.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
No description provided.