Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still a couple of critical issues that break the required modifier-driven layout behavior.
- Calendar width calculation (critical layout requirement)
Insrc/styles/main.scss, your calendar width is currently something like:
width: calc((#{$cell-size} * 7) + (#{$gap} * 6));
The task explicitly requires the total width to be 7 columns + 10px paddings, but this calc does not include the calendar’s left/right padding. You need to either:
- Add padding to the formula, e.g.
+ (#{$padding} * 2), or - Adjust
box-sizingand/or how you setwidth/paddingso that the final rendered width (content + gaps + paddings) exactly matches the spec.
Right now this requirement is not met, so the layout sizing is incorrect.
-
Start-day mapping and modifier behavior (critical checklist requirement)
Insrc/styles/main.scssyou defined the map of days as:
$days: (mon: 0, tue: 1, wed: 2, thu: 3, fri: 4, sat: 5, sun: 6);
but your HTML grid is built with Sunday in the first column. That means the correct indices must match the visual order:
sun: 0, mon: 1, tue: 2, wed: 3, thu: 4, fri: 5, sat: 6.
Because you then usemargin-left: calc((#{$cell-size} + #{$gap}) * #{$index}), the wrong mapping causes modifiers likecalendar--start-day-monorcalendar--start-day-frito place day 1 in the wrong weekday column. This violates the requirement that changing thestart-daymodifier should correctly change the calendar layout ([CHECKLIST ITEM #1]). Update the map so that indices line up with the actual column order. -
Multiple
start-daymodifiers in HTML (BEM and behavior issue)
Insrc/index.html, your calendar element currently has two start-day modifiers, for example:
class="calendar calendar--start-day-sun calendar--start-day-wed ...".
Having multiple modifiers that control the same aspect (start-day) on one block is a BEM anti-pattern and makes the layout unpredictable. It also breaks the requirement that changing a singlestart-daymodifier should clearly update the layout ([CHECKLIST ITEM #1] and BEM “Typical Mistakes”). You should keep only onecalendar--start-day-*class on the calendar (for default behavior, keepcalendar--start-day-sunand remove the others).
Once you fix the width calc to include padding and correct both the day-index mapping and the duplicate start-day modifiers, your solution will satisfy the functional and checklist requirements. The rest of your work (BEM structure, SCSS variables, @for loops, hover effects, and month-length handling) looks solid—these changes will bring everything in line with the task expectations.
✨ 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
src/styles/main.scss
Outdated
| padding: $padding; | ||
|
|
||
| // Logic: 7 columns * size + 6 gaps | ||
| width: calc((#{$cell-size} * 7) + (#{$gap} * 6)); |
There was a problem hiding this comment.
Width calc omits calendar padding. The task requires: "Use flex with 1px gap and limit its width to exactly 7 columns + 10px paddings" — your current calc is calc((#{$cell-size} * 7) + (#{$gap} * 6)) which does not include the left/right padding of the calendar. Include padding in the calculation (for example add + (#{$padding} * 2)) or change box-sizing/width handling so the final rendered width equals 7 columns + paddings.
src/styles/main.scss
Outdated
| $days: ( | ||
| mon: 0, | ||
| tue: 1, | ||
| wed: 2, | ||
| thu: 3, | ||
| fri: 4, | ||
| sat: 5, | ||
| sun: 6, | ||
| ); | ||
|
|
||
| @each $day, $index in $days { | ||
| // OLD: &--start-#{$day} -> creates .calendar--start-sun | ||
| // NEW: &--start-day-#{$day} -> creates .calendar--start-day-sun | ||
| &--start-day-#{$day} { | ||
| .calendar__day:first-child { | ||
| margin-left: calc((#{$cell-size} + #{$gap}) * #{$index}); |
There was a problem hiding this comment.
The start-day mapping is incorrect for a calendar that is built with Sunday as the first column in HTML. You defined $days: (mon: 0, tue: 1, wed: 2, thu: 3, fri: 4, sat: 5, sun: 6) and then apply margin-left: calc((#{$cell-size} + #{$gap}) * #{$index}). With the HTML starting from Sunday, the correct mapping should reflect Sunday at index 0, Monday at index 1, Tuesday 2, etc. For example: sun: 0, mon: 1, tue: 2, wed: 3, thu: 4, fri: 5, sat: 6. As-is, modifiers like calendar--start-day-mon or calendar--start-day-fri will not place the 1st on the correct weekday column. This also means you currently violate the checklist item: "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout" — please correct the mapping so modifier changes produce the expected layout.
src/index.html
Outdated
| <body> | ||
| <h1>Calendar</h1> | ||
| <div | ||
| class="calendar calendar--start-day-sun calendar--start-day-wed calendar--month-length-31" |
There was a problem hiding this comment.
The calendar element includes two start-day modifiers at once: calendar--start-day-sun calendar--start-day-wed. Having multiple start-day modifiers on the same block is a BEM mistake and makes modifier-driven layout unpredictable. Remove the extra modifier and keep only one calendar--start-day-* class (the task asks to "Set calendar to start from Sunday by default in your HTML"). This violates the checklist item: "- [ ] Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout" and also violates: "- [ ] All Typical Mistakes from BEM lesson theory are checked." Update the class list so only a single start-day modifier is present.
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 functional mismatches with the task requirements. First, the $start-days map in src/styles/main.scss assumes Monday is the first column (mon: 1, ... sun: 7), but your calendar visually starts with Sunday, so the calendar--start-day-* modifiers do not place day 1 under the correct weekday column [CHECKLIST ITEM #1]. Update the map so the numeric index matches the visual column order (Sunday as the first column), for example: sun: 1, mon: 2, ..., sat: 7 (or use a 0-based scheme and adjust your margin-left calculation accordingly). Second, your width calculation currently adds 2 * $padding while $padding itself is 10px, which yields 20px of total horizontal padding, but the requirement says the calendar width must be exactly “7 columns + 10px paddings” in total; either reduce $padding to 5px or change the width formula so left+right padding sum to 10px. Once you fix the start-day mapping so modifier changes correctly shift the first day, and adjust the width so total padding is exactly 10px, your solution will meet the remaining core requirements. Overall, the rest of your implementation (SCSS structure, BEM usage, hover behavior, month-length modifiers, and numbering via ::before) looks solid—these last two corrections will complete the task nicely.
✨ 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
src/styles/main.scss
Outdated
| $start-days: ( | ||
| mon: 1, | ||
| tue: 2, | ||
| wed: 3, | ||
| thu: 4, | ||
| fri: 5, | ||
| sat: 6, | ||
| sun: 7, | ||
| ); |
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 $start-days map currently maps mon..sun to 1..7 but your calendar grid uses Sunday as the first column, so calendar--start-day-sun must produce zero left offset. Update the map so the index aligns with the visual order (Sunday first). For example: sun: 1, mon: 2, tue: 3, wed: 4, thu: 5, fri: 6, sat: 7 (or use 0-based indices and adjust the calc).
src/styles/main.scss
Outdated
| $padding: 10px; | ||
| $cell-step: $cell-size + $gap; | ||
| $day-bg: #eee; | ||
| $border-color: #000; | ||
|
|
||
| body { | ||
| margin: 0; | ||
| min-height: 100vh; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| } | ||
|
|
||
| .calendar { | ||
| padding: $padding; | ||
|
|
||
| display: flex; | ||
| flex-wrap: wrap; | ||
| gap: $gap; | ||
|
|
||
| width: calc( | ||
| #{$columns} * #{$cell-size} + (#{$columns} - 1) * #{$gap} + 2 * #{$padding} | ||
| ); |
There was a problem hiding this comment.
The requirement states the calendar width must equal "7 columns + 10px paddings" but your current code sets $padding: 10px and then adds 2 * $padding in the width calc, which yields 20px total padding. Make the total left+right padding equal 10px (e.g. set $padding: 5px or change the calc to add a single $padding value) so the computed width is exactly 7 columns + 10px paddings.
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, but there are two important layout issues to fix next time. You did a solid job with the SCSS structure: BEM naming, use of @for with ::before for day numbers, @each for start-day modifiers, and nth-child for month-length all meet the task requirements. The first critical issue is the weekday mapping in src/styles/main.scss: your $week-days map (around lines 11–17) uses mon: 1, ... sun: 7, but the calendar visually starts with Sunday (calendar--start-day-sun), so changing .calendar--start-day-* does not place day 1 under the correct weekday column [CHECKLIST ITEM #1]; you can fix this by reordering the map so sun is position 1 (or by adjusting the margin calculation to match your numbering). The second critical issue is the container width/padding: you set $padding: 10px (line 4) and then use + 2 * $padding in $container-width (line 9), which produces 20px total horizontal padding instead of the required single 10px total; you can either change $padding to 5px or change the width formula to add only 10px total. Aside from these, the hover behavior, transitions, and general layout are nicely implemented and show good understanding of the requirements. Keep going—resolving these two details will bring your calendar fully in line with the spec and strengthen your control over layout math in CSS.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| mon: 1, | ||
| tue: 2, | ||
| wed: 3, | ||
| thu: 4, | ||
| fri: 5, | ||
| sat: 6, | ||
| sun: 7, |
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 $week-days map currently maps mon: 1, ... sun: 7 (lines 11–17), but your HTML uses calendar--start-day-sun as default. Because you calculate margin-left: ($pos - 1) * ($day-size + $gap) for the first day (line 77), the numeric positions must match the visual column order (Sunday should be position 1). Update the map so it follows the visual columns (for example sun: 1, mon: 2, tue: 3, wed: 4, thu: 5, fri: 6, sat: 7) or use a 0-based scheme and adjust the margin formula accordingly so changing .calendar--start-day-* classes moves the first day to the correct column.
| $padding: 10px; | ||
| $font-size: 30px; | ||
| $bg: #eee; | ||
| $hover: #ffbfcb; | ||
| $col-width: $day-size; | ||
| $container-width: 7 * $col-width + 6 * $gap + 2 * $padding; |
There was a problem hiding this comment.
The width calculation makes total horizontal padding = 2 * $padding (you set $padding: 10px on line 4 and use + 2 * $padding in $container-width on line 9), which yields 20px total horizontal padding. The requirement states the calendar width must be exactly "7 columns + 10px paddings". Change either $padding to 5px (so left+right = 10px) or modify $container-width to add only 10px total (e.g. + 10px instead of + 2 * $padding) so the total horizontal padding equals 10px.
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 ❗️