add task solution#5481
Conversation
Saint17404
commented
Jul 14, 2026
Saint17404
commented
- DEMO LINK
- TEST REPORT LINK
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 of the task is not fully met: the calendar block itself must be centered in the middle of the screen both horizontally and vertically. Right now, the centering is implemented on the body as a flex container, while the .page wrapper around .calendar has no styles and does not participate in any centering logic, so the calendar is not clearly treated as the main centered BEM block as the specification intends.
Critical issue to fix (blocker):
- In
src/styles/main.scss, move the centering responsibility frombodyto a dedicated BEM block (either.pageor.calendar). For example, you can make.pagea full-height flex container (min-height: 100vh; display: flex; justify-content: center; align-items: center;) that directly centers.calendar, or remove.pageand treat.calendaras the centered block. This will ensure the calendar component itself is what’s explicitly centered on the screen per the requirement.
Everything else (SCSS usage, BEM structure, modifiers for start-day and month-length, hover behavior, and layout logic) looks solid and aligns well with the task. Once you adjust the layout so the calendar block is the explicitly centered element, your solution should fully satisfy the task description. Great work so far—this is a small but important layout correction to finish it off.
✨ 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
| </head> | ||
| <body> | ||
| <h1>Calendar</h1> | ||
| <div class="page"> |
There was a problem hiding this comment.
The task requires: "Display a calendar in the middle of the screen (both horizontally and vertically)." Right now body is the flex container, but .page (the wrapper around .calendar) has no styles, so the calendar block itself is not explicitly centered as a BEM block. Consider adding styles for the .page (or moving the centering to .calendar as the main block) to meet this requirement more explicitly.
| $padding: 10px; | ||
| $days-in-week: 7; | ||
| $border-color: black; | ||
| $background-color: #eee; |
There was a problem hiding this comment.
The requirement says: "Display a calendar in the middle of the screen (both horizontally and vertically)", but currently the body is centered as a flex container and the .page wrapper has no styles, so the calendar block itself is not explicitly centered as a component. Consider either styling the .page block (e.g., making it a full-height flex container centering .calendar) or removing the extra wrapper and treating .calendar as the main centered block to better match the requirement.
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 layout requirement is still not met: the calendar must be centered via a dedicated BEM block, not by styling the body element. Functionally, your calendar behaves correctly (31 days, dynamic start-day and month-length modifiers, hover behavior, SCSS loops and variables), but the centering responsibility is explicitly required to live on a block like .page or .calendar, so this remains a critical blocker. To fix this, remove the flex-centering styles from body in src/styles/main.scss (e.g., display: flex, justify-content: center, align-items: center, min-height: 100vh if present). Then introduce a wrapper block such as .page in index.html around your .calendar, and apply those centering styles to .page instead, or apply them directly to .calendar if you prefer that block to be the centered element. This change ensures that the calendar component itself is the explicitly centered block, as required and as noted in the previous review and in the task description. Once you adjust the centering to use a proper BEM block and keep the calendar vertically and horizontally centered, your solution will satisfy all the core requirements. Overall, your use of SCSS and BEM is strong; this is mainly about aligning the layout structure with the specified conventions, and you’re very close to a fully compliant implementation.
✨ 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
| body { | ||
| margin: 0; | ||
| min-height: 100vh; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| } |
There was a problem hiding this comment.
The task and previous review require centering the calendar via a dedicated BEM block (e.g., .page or .calendar), not by styling body. This still violates the requirement: the calendar component itself must be the explicitly centered block, so move this flex centering from body to a BEM block.
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 — the calendar meets the functional and structural requirements very well. The calendar is correctly centered using a .page wrapper with flexbox, the markup has exactly 31 .calendar__day elements with no hardcoded numbers, and day numbers are injected via SCSS loops using ::before, which aligns with the expectations. Your use of SCSS variables, @for and @each loops, and BEM methodology (.calendar block, .calendar__day element, modifiers only on the block) is clean and demonstrates solid understanding, satisfying [CHECKLIST ITEM #1] and [CHECKLIST ITEM #2]. The start-day and month-length modifiers work as intended, correctly shifting the first day and hiding extra days, and the hover effect (pointer, pink background, smooth upward movement) matches the required interaction. I don’t see critical issues that need correction for this task, so no mandatory changes are required. Overall, this is a strong, well-structured implementation — keep up this level of clarity and attention to detail in future tasks.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
