calendar layout#5457
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 a few core task requirements and checklist items are not yet satisfied, which makes the solution incomplete. The styles are implemented in src/styles/index.scss and linked as styles/index.scss, but the task explicitly requires using src/styles/main.scss as the stylesheet entry and linking that file in index.html, so this must be corrected. The calendar block in index.html only uses calendar--start-day-sun and never applies a month-length modifier (e.g. calendar--month-length-31), and your SCSS currently defines .calendar--month-#{$month} instead of the required calendar--month-length-XX pattern, so changing month-length in HTML cannot affect the layout as required [CHECKLIST ITEM #1]. You also hardcode width: 726px; on the calendar, whereas the requirement is to derive the width from variables (day size, gap, paddings) so it explicitly equals 7 columns plus 10px paddings, rather than using a fixed pixel value. To fix the critical issues, move/rename your SCSS to main.scss and update the <link> in index.html, rename the month modifier to include month-length and apply a default calendar--month-length-31 class in HTML, and compute the calendar width using SCSS variables instead of a hardcoded 726px. As minor cleanups, remove unused/non‑BEM classes like .start__day and .month__day, and drop the redundant .calendar__day:first-child::before rule since the @for loop already assigns the day numbers. Once these changes are made, your solution will align well with the task requirements and your existing correct parts (hover behavior, numbering, start-day handling) will fully pass the checklist.
✨ 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
| <link | ||
| rel="stylesheet" | ||
| href="styles/index.scss" |
There was a problem hiding this comment.
The task description explicitly requires writing styles in src/styles/main.scss and using that as the entry; here you’re linking to styles/index.scss instead, so the main stylesheet file name/path does not match the specified requirement.
| /> | ||
| </head> | ||
| <body> | ||
| <div class="calendar calendar--start-day-sun"> |
There was a problem hiding this comment.
The root calendar element only has calendar--start-day-sun and lacks any month-length modifier; this violates checklist item #1: Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout, because there is no month-length modifier applied here to control the visible number of days.
| data-day="2" | ||
| ></div> | ||
| <div |
There was a problem hiding this comment.
The calendar width is hardcoded to 726px; per the description you should base this on variables (day width, gap, padding) instead of repeating hardcoded pixel values so calculations are explicit and maintainable.
| ></div> | ||
| <div | ||
| class="calendar__day" | ||
| data-day="8" |
There was a problem hiding this comment.
The selector .start__day is not part of the required BEM structure and isn’t used anywhere; consider removing it to avoid unused, non-BEM-consistent code.
| ></div> | ||
| <div |
There was a problem hiding this comment.
This content: "1"; for the first child is redundant because the @for loop below already assigns numbers 1–31 via ::before; keeping both can be confusing and is unnecessary.
| .calendar { | ||
| flex-wrap: wrap; | ||
| display: flex; | ||
| gap: 1px; | ||
| padding: 10px; | ||
| width: 726px; |
There was a problem hiding this comment.
Hardcoding width: 726px; breaks the requirement to "use flex with 1px gap and limit its width to exactly 7 columns + 10px paddings" using variables and calculations instead of fixed px values. Consider deriving this from $day-width, $gap, and padding variables.
| } | ||
| } | ||
|
|
||
| .start__day { |
There was a problem hiding this comment.
The .start__day class is not BEM-compliant and is unused in HTML. Keeping non-BEM, unused classes conflicts with checklist item #3 about avoiding typical BEM mistakes.
| } | ||
|
|
||
|
|
||
| .calendar__day:first-child::before { |
There was a problem hiding this comment.
This rule is redundant because the @for loop below already assigns content for each calendar__day including the first one. Having both can cause confusion and is unnecessary.
| } | ||
|
|
||
|
|
||
| .month__day { |
There was a problem hiding this comment.
The .month__day class is unused and not part of the required BEM structure. Leaving such stray classes conflicts with the checklist requirement to adhere to BEM and avoid typical mistakes.
| } | ||
| } | ||
|
|
||
| @for $month from 28 through 31 { |
There was a problem hiding this comment.
Your @for creates .calendar--month-#{$month}, but the spec requires a month-length modifier (e.g. calendar--month-length-28). This mismatch means the required month-length modifier (checklist item #1) is not implemented as described or used in HTML.
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 meets all the functional and structural requirements for the calendar task. The calendar is correctly centered, uses flexbox with a 1px gap, and its width is computed via SCSS variables to create exactly seven columns with the specified paddings. Day numbers are generated purely via SCSS using @for and ::before, and the HTML has exactly 31 .calendar__day elements with no hardcoded numbers, satisfying the core constraints and [CHECKLIST ITEM #2]. The calendar--start-day-* and calendar--month-length-* block modifiers are implemented with @each and @for, and changing these modifiers in the HTML properly shifts the first day and hides extra days as required by [CHECKLIST ITEM #1]. Hover behavior (pointer cursor, pink background #FFBFCB, transform: translateY(-20px), 0.5s transition) matches the task exactly.
One small non-blocking improvement you might consider is replacing the literal height: 100px with the $day-width variable for consistency and easier future changes. Overall, your BEM naming, SCSS structure, and behavior all align well with the task description—nice work on implementing the modifiers and dynamic layout cleanly.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.