Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are a few critical points where the implementation does not yet match the explicit task requirements and they affect core functionality. First, the $days map in src/styles/blocks/variables.scss (and its usage in calendar.scss) assumes a Monday-first week, so calendar--start-day-sun currently shifts the first day into the last column; you need to remap it so 'sun': 0, 'mon': 1, 'tue': 2, ... 'sat': 6 and ensure the margin calculation follows this mapping, so changing the start-day modifier correctly moves the first day’s column. Second, the calendar width is set to $size while padding is added separately, so the final outer width becomes $size + 2 * $padding; adjust the calculation (either include padding inside the width variable or explicitly set width: $size + $padding * 2) so the rendered width equals exactly 7 columns plus the 10px paddings as required. Third, the main stylesheet is named src/styles/index.scss and the HTML links to styles/index.scss, but the task explicitly requires src/styles/main.scss and a <link> to styles/main.scss; please rename/move the file and update the HTML reference accordingly. As a small improvement, consider renaming the $size variable to something more descriptive like $calendar-inner-width to make your layout calculations clearer, but this is secondary to the three mandatory fixes above. Once you address these points, the calendar will both visually and semantically match all stated requirements, and the solution will be in great shape.
✨ 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, $i in $days { |
There was a problem hiding this comment.
This violates the checklist requirement: "- [ ] Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout" — the start-day mapping is wrong. The $days map uses 'mon': 0, 'tue': 1, ..., 'sun': 6, which assumes the first column is Monday. Since the calendar in HTML is set to start from Sunday by default, the mapping should make sun => 0, mon => 1, tue => 2, ... so margin-left places the first day under the correct weekday. Update the $days map (or the multiplication) so .calendar--start-day-sun results in margin-left: 0, .calendar--start-day-mon results in margin-left: 1 * (cell + gap), etc.
src/styles/blocks/calendar.scss
Outdated
| .calendar { | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| width: $size; |
There was a problem hiding this comment.
This violates the implementation detail: "Use flex with 1px gap and limit its width to exactly 7 columns + 10px paddings" — you set width: $size; but $size is calculated without including the padding. Because you apply padding: $padding;, the total outer width becomes $size + 2 * $padding. To match the requirement, include padding in the width calculation (for example compute $size including + $padding * 2) or set width: $size + $padding * 2 so the final width equals 7 columns plus the 10px paddings.
| @@ -1,3 +1,12 @@ | |||
| @import './blocks/variables'; | |||
There was a problem hiding this comment.
This violates the requirement: "Write styles in src/styles/main.scss instead of src/style.css." The stylesheet here is named index.scss (src/styles/index.scss). Rename this file to main.scss (or place these contents into src/styles/main.scss) and update the HTML link so the main stylesheet path exactly matches the task description.
| </head> | ||
| <body> | ||
| <h1>Calendar</h1> | ||
| <div class="calendar calendar--start-day-sun calendar--month-length-31"> |
There was a problem hiding this comment.
This violates the requirement from the task description: "Write styles in src/styles/main.scss instead of src/style.css." The HTML currently links to styles/index.scss (line 12). Please rename/move your stylesheet to src/styles/main.scss (or create main.scss and import the content) and update the <link> in src/index.html to href="styles/main.scss" so the project matches the exact task requirement.
src/styles/blocks/variables.scss
Outdated
| $padding: 10px; | ||
| $gap: 1px; | ||
| $cell-size: 100px; | ||
| $size: $cell-size * $count-day + $gap * ($count-day - 1); |
There was a problem hiding this comment.
The $size variable name is ambiguous. The task requires: "Use properly named variables to make all the calculations more clear." Consider renaming $size to something like $columns-width or $calendar-inner-width to make its purpose explicit (it represents combined width of 7 cells and gaps).
| $days: ( | ||
| 'mon': 0, | ||
| 'tue': 1, | ||
| 'wed': 2, | ||
| 'thu': 3, | ||
| 'fri': 4, | ||
| 'sat': 5, | ||
| 'sun': 6, |
There was a problem hiding this comment.
This mapping will break the start-day modifiers and thus violates the checklist item: "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout". The $days map currently sets 'mon': 0 through 'sun': 6, which makes calendar--start-day-sun offset the first day to the last column. To match the HTML default calendar--start-day-sun (first day placed in the leftmost column), change the map so 'sun': 0, 'mon': 1, 'tue': 2, 'wed': 3, 'thu': 4, 'fri': 5, 'sat': 6.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still two critical layout issues that break the required behavior and checklist items. First, in src/styles/blocks/variables.scss the $days map currently treats Monday as index 0 and Sunday as 6, but the task expects Sunday to be column 0, Monday column 1, etc.; this causes modifiers like calendar--start-day-sun to place the first day in the last column instead of the first, violating “Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout” [CHECKLIST ITEM #1]. Fix this either by remapping $days to 'sun': 0, 'mon': 1, 'tue': 2, 'wed': 3, 'thu': 4, 'fri': 5, 'sat': 6 in variables.scss, or by adjusting the margin-left formula in calendar.scss to use ($i - 1) * ($cell-size + $gap) so indices match the expected columns. Second, the calendar width currently excludes padding in variables.scss while calendar.scss sets both width: $calendar-width; and padding: $padding;, so the final outer width becomes cells + gaps + 2 * padding, which is larger than “7 columns + 10px paddings” and breaks that requirement. To fix this, either include + $padding * 2 in the width variable (e.g. compute a $calendar-outer-width and use it in width) or adjust the width expression directly so that the resulting outer width exactly equals 7 cell widths + 6 gaps + 2×10px padding. Everything else (markup structure, day count, numbering via ::before, hover effects, flex layout) looks good, so once these two blocking issues are corrected your solution should 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
| $days: ( | ||
| 'mon': 0, | ||
| 'tue': 1, | ||
| 'wed': 2, | ||
| 'thu': 3, | ||
| 'fri': 4, | ||
| 'sat': 5, | ||
| 'sun': 6, |
There was a problem hiding this comment.
This violates checklist item #1: "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout". The $days map currently treats Monday as column 0 and Sunday as column 6, so .calendar--start-day-sun will shift the first day into the last column instead of making Sunday the first column. Remap the map to 'sun': 0, 'mon': 1, 'tue': 2, 'wed': 3, 'thu': 4, 'fri': 5, 'sat': 6 or change the margin calculation to use ($i - 1) * ($cell-size + $gap) so each start-day modifier moves the first day to the correct column.
| $padding: 10px; | ||
| $gap: 1px; | ||
| $cell-size: 100px; | ||
| $calendar-width: $cell-size * $count-day + $gap * ($count-day - 1); |
There was a problem hiding this comment.
This violates the requirement: "Limit calendar width to exactly 7 columns + 10px paddings." $calendar-width currently sums only cells and gaps, but padding: $padding is applied later which makes the element's outer width equal to $calendar-width + 2 * $padding. Include padding in the width calculation (for example $calendar-width: $cell-size * $count-day + $gap * ($count-day - 1) + $padding * 2;) or create a separate $calendar-outer-width used for width so the final rendered outer width equals 7 columns plus the 10px paddings.
| width: $calendar-width; | ||
| gap: $gap; | ||
| padding: $padding; |
There was a problem hiding this comment.
This breaks the requirement: "Limit calendar width to exactly 7 columns + 10px paddings." You set width: $calendar-width; and padding: $padding; on the same element. Because $calendar-width (defined in variables) already accounts for cells and gaps, adding padding makes the final outer width larger than the intended value. Include padding in the width calculation (for example compute an $calendar-outer-width that adds 2 * $padding to the inner width) or use the correct expression so the rendered outer width equals exactly 7 columns plus the required paddings. (See width and padding here.)
| @each $day, $i in $days { | ||
| .calendar--start-day-#{$day} .calendar__day:first-child { | ||
| margin-left: $i * ($cell-size + $gap); | ||
| } | ||
| } |
There was a problem hiding this comment.
This risks violating the checklist item: "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout". The @each block uses the $days map value $i directly to compute margin-left: $i * ($cell-size + $gap);. That works only if the $days map indexes match the expected 0-based column positions (Sunday = 0, Monday = 1, ..., Saturday = 6). Currently the project's $days map is Monday-first (so Sunday maps to 6), which will shift calendar--start-day-sun to the last column instead of the first. Fix by remapping $days to 'sun': 0, 'mon': 1, ... in variables.scss or change the formula to account for a 1-based index (e.g. ($i - 1) * ...). (See the @each and margin-left here.)
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 ❗️