Add calendar layout with SCSS#5441
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 not fully met: the calendar block itself is not reliably centered in the middle of the viewport. The task explicitly requires the calendar to be centered both horizontally and vertically on the screen, but currently body is a flex container without a guaranteed full-height layout, so on some setups the calendar may not be perfectly centered in the viewport. To fix this, ensure the centering container spans the viewport (for example, by adding html, body { height: 100%; } or using a full-height wrapper with min-height: 100vh) and keep .calendar as the flex item being centered. In addition, there is a failing SCSS lint rule at src/styles/main.scss:19:81 due to a newline after the * operator; rewrite that calculation so the * is not followed by a line break (e.g., width: $cell-width * -1;). Everything else in your solution—SCSS usage, BEM naming, modifiers, hover behavior, and dynamic layout via @for/@each—looks solid, so once you fix the viewport centering and the small lint issue, your implementation will fully satisfy the task requirements.
✨ 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="calendar calendar--start-day_sun calendar--month-length_31"> |
There was a problem hiding this comment.
The task requires: "Display a calendar in the middle of the screen (both horizontally and vertically)." With the current styles, body is a flex container that centers its children, but the calendar is the only child and there is no margin or centering applied to it specifically; in some setups this can cause the calendar not to appear perfectly centered relative to the viewport. Consider ensuring the .calendar block itself is centered in the viewport (for example by confirming body spans the viewport and .calendar is the flex item being centered) or by explicitly using a container that fills the screen and centers .calendar within it.
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 now meets all functional and technical requirements of the task. The calendar is correctly centered in the viewport using full-height html, body and a flexbox body, and the markup contains exactly 31 .calendar__day elements with no hardcoded numbers or modifiers on the days themselves. You correctly use SCSS with BEM naming, variables for layout (day size, border, gap, padding, columns), and generate both the day numbers (via ::before and @for) and the start-day / month-length modifiers (via @each and @for) in a clean, maintainable way. Hover behavior matches the specification precisely: pointer cursor, pink #FFBFCB background, transform: translateY(-20px), and transition: all 0.5s. Changing calendar--start-day_* and calendar--month-length_* on the block correctly updates the layout, and days beyond the selected month length are hidden as required [CHECKLIST ITEM #1]. You also resolved the previous SCSS linter issue with the * operator, so there are no blocking style problems left. Nicely done bringing everything in line with the checklist and requirements; this is a solid, spec-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
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can keep progressing while you learn. Your HTML and SCSS implementation is very strong: the calendar is correctly centered with flex, you have exactly 31 .calendar__day elements, the BEM naming is clean, numbers are generated via ::before and @for, and the hover effect (pointer, #FFBFCB, translateY(-20px), 0.5s transition) matches the requirements. One issue shown by the visual tests is that the dynamic classes for configuration are not being applied as expected: the tests wait for .calendar--start-day-sun and .calendar--month-length-31, so make sure your calendar container actually has class="calendar calendar--start-day-sun calendar--month-length-31" in the final rendered HTML or is given these classes via JS based on its data-* attributes. This matters because those modifier classes control which column the month starts on and how many days are visible, which is a core part of the behavior. As an improvement, double-check that your class naming uses the same separator style everywhere (calendar--start-day_sun vs calendar--start-day-sun) so the CSS selectors match what the tests expect [CHECKLIST ITEM #X]. Overall, you’ve done a great job with SCSS structure and layout logic—fixing the class naming/assignment so it matches the test expectations will make your solution both visually correct and fully testable.
✨ 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
2 participants
DEMO LINK
TEST REPORT LINK
[x] Changing 'month-lengh' and 'start-day' modifier in the code element
reflects in changing calendar layout
[x] Each day has no modifiers, only class (eg. calendar__day)
[x] All
Typical MistakesfromBEMlesson theory are checked.[x] Code follows all the Code Style Rules ❗️