solution: layout calendar#5461
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 there is still a functional mismatch between the required weekday alignment and your current implementation. The $weekdays map in src/styles/utils/variebles.scss / calendar.scss currently makes Monday index 0 and Sunday index 6, which causes calendar--start-day-mon and other modifiers to place the first day in the wrong visual column (e.g., Monday ends up in column 0 instead of column 1, Friday not in column 5), violating the requirement “Monday is the 1st, Friday is the 5th” and [CHECKLIST ITEM #1]. You should adjust the map so that the visual columns correspond correctly, for example ensuring that when calendar--start-day-mon is applied, the first day appears in the second column, and when calendar--start-day-fri is applied, it appears in the fifth column; this will likely mean updating the indices in $weekdays and verifying the margin-left calculation on .calendar__day:first-child in src/styles/blocks/calendar.scss.
The misspelled variables file name (variebles.scss) and its import path ./utils/variebles in src/styles/utils/mixin.scss are not strictly blocking, but they do conflict with code style expectations; renaming it to variables.scss and updating the import will improve clarity and better align with [CHECKLIST ITEM #4]. Everything else in your solution—SCSS structure, BEM usage, hover behavior, month-length modifiers, and general layout—is very well done, so once the weekday mapping and resulting column alignment are corrected, your calendar will fully meet 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
|
|
||
| @each $day, $index in $weekdays { | ||
| &--start-day-#{$day} &__day:first-child { | ||
| margin-left: $index * ($calendar-day-size + $calendar-space-size); | ||
| } | ||
| } | ||
|
|
||
| @for $length from 28 through 31 { | ||
| &--month-length-#{$length} &__day:nth-child(n + #{$length + 1}) { | ||
| display: none; | ||
| } | ||
| } |
There was a problem hiding this comment.
The current $weekdays map ('mon': 0, 'tue': 1, ..., 'sun': 6) makes Monday start at column 0 instead of column 1, so for calendar--start-day-mon the first day is not rendered in the 2nd column as required (“The month should start at the correct column (Monday is the 1st, Friday is the 5th)”). Adjust the indices so that the visual columns match the required positions.
| @@ -0,0 +1,5 @@ | |||
| @mixin flex-center { | |||
There was a problem hiding this comment.
The import path ./utils/variebles references a misspelled variables file; while it works, this naming hurts clarity and may conflict with the code style guideline (Checklist item #4); consider renaming it to variables and updating imports.
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 fully implements the required calendar behavior and matches all checklist items. The calendar is correctly centered with flex, uses 31 .calendar__day elements without numbers in the HTML, and injects day numbers via SCSS using ::before and @for as required. The calendar--start-day-* and calendar--month-length-* modifiers work properly for all specified values: changing them in HTML correctly shifts the first day to the proper column (e.g., Monday in the 2nd column, Friday in the 5th) and hides extra days with nth-child, satisfying the functional rules and [CHECKLIST ITEM #1]. You also respected BEM conventions, kept only the calendar__day class on each cell (no modifiers on days), and organized variables/mixins cleanly, which aligns with [CHECKLIST ITEMS #2–#4].
There are no critical or blocking issues remaining from the task’s perspective; any minor style or lint findings can be handled by automated tools. Overall, this is a solid, well-structured implementation that closely follows the specification and makes good use of SCSS features.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
DEMO LINK
TEST REPORT LINK
Changing 'month-lengh' and 'start-day' modifier in the code element
reflects in changing calendar layout
Each day has no modifiers, only class (eg. calendar__day)
All
Typical MistakesfromBEMlesson theory are checked.Code follows all the Code Style Rules ❗️