Skip to content

First promises#1558

Open
balakhmei wants to merge 2 commits into
mate-academy:masterfrom
balakhmei:develop
Open

First promises#1558
balakhmei wants to merge 2 commits into
mate-academy:masterfrom
balakhmei:develop

Conversation

@balakhmei

Copy link
Copy Markdown

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review Summary

Your solution is approved because all core functional requirements from the task are correctly implemented. You declare both promise1 and promise2 in src/scripts/main.js, resolve promise1 on the .logo click event, and reject promise2 via setTimeout after 3000 ms, matching the specification. Both promises have .then and .catch handlers, and these handlers append the required <div class="message"> and <div class="message error-message"> elements with the appropriate text to the body, which aligns with the Cypress tests. There are no critical blockers or broken functionality evident in the code, so the implementation should pass the automated tests for the described behavior.

As minor, non-blocking suggestions, you could consider extracting shared logic for creating and appending message elements so promise1 and promise2 reuse the same helper functions, which would reduce duplication. Also, note that promise1 never actually rejects, so the .catch there is mostly illustrative; if you keep it, you might document that it’s not expected to run. Finally, please double-check that your demo link in the README or PR description includes your GitHub username as required, and that you’ve run the specified npm test commands locally to fully satisfy the process checklist. Overall, nice job implementing the promises and wiring them up cleanly to the DOM and tests.


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

Footnotes

  1. Rate AI review example

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