Tasks solved#5475
Conversation
GrizlixXx
commented
Jul 10, 2026
GrizlixXx
commented
- DEMO LINK
- TEST REPORT LINK
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 are still a few critical functional issues that conflict with the task requirements and checklist. First, your calendar width variable currently accounts only for 7 day cells and gaps and does not include the horizontal paddings ($calendar-padding: 10px), so the final rendered width is not “exactly 7 columns + 10px paddings” as required; update $calendar-width (or the width + padding combination) to include both left and right paddings using the padding variable. Second, the @for loop that generates calendar--month-length-* modifiers uses a range that excludes 31, so the calendar--month-length-31 class in HTML has no effect and breaks the requirement that changing month-length modifiers should change the layout [CHECKLIST ITEM #1]; adjust the upper bound so 28–31 are all generated. Third, your $week-days map currently assigns mon: 0, tue: 1, etc., which places Monday in the first column instead of Sunday and contradicts “calendar starts from Sunday by default” and the described column positions; update these offsets so Sunday is treated as the first column and the margins align accordingly. Finally, you only set transition-duration on .calendar__day, so the hover changes to background-color and transform are not explicitly animated; define a full transition (e.g. transition: background-color $anim-duration, transform $anim-duration;) so both properties animate over 0.5s as specified. Once you fix the width formula, month-length loop range, weekday offsets, and hover transition, your solution will match the functional and layout requirements very well—most of your structure, SCSS usage, and BEM naming are already solid, so you are close to completion.
✨ 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
| @@ -9,10 +13,43 @@ | |||
| <title>Calendar</title> | |||
| <link | |||
| rel="stylesheet" | |||
There was a problem hiding this comment.
The current $calendar-width only accounts for 7 day squares and 6 gaps, but ignores the horizontal padding applied on .calendar. The task requires limiting the width to exactly 7 columns plus 10px paddings, so you should include $calendar-padding in the width calculation to match the total layout width requirement.
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> |
There was a problem hiding this comment.
You set only transition-duration on .calendar__day, but the requirement says all hover changes (background-color and transform) should be animated. Consider defining the full transition (including properties like background-color and transform) to ensure both effects are animated over 0.5s.
| .calendar { | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| width: $calendar-width; |
There was a problem hiding this comment.
The calendar width variable currently only accounts for 7 day cells and gaps, but the task requires the width to be "exactly 7 columns + 10px paddings"; update $calendar-width (or the combination of width and padding) so that the final box width also includes the horizontal paddings defined by $calendar-padding.
| padding: $calendar-padding; | ||
| gap: $calendar-gap; | ||
|
|
||
| @for $length from 28 through 31 { |
There was a problem hiding this comment.
This @for loop uses through 31, but in Sass that upper bound is exclusive, so you never generate a &--month-length-31 modifier; this breaks checklist item "Changing 'month-lengh' ... reflects in changing calendar layout" because calendar--month-length-31 in HTML has no effect.
| $day-hover-color: #ffbfcb; | ||
| $day-hover-transform: translateY(-20px); | ||
| $anim-duration: 0.5s; | ||
| $calendar-width: (7 * $day-size) + 6 * $calendar-gap; |
There was a problem hiding this comment.
The calendar width is currently 7 * $day-size + 6 * $calendar-gap, but the description requires limiting width to exactly 7 columns plus 10px paddings. Since you already have $calendar-padding: 10px, consider including both left and right paddings in $calendar-width so the total width accounts for them and matches the requirement.
| $week-days: ( | ||
| mon: 0, | ||
| tue: 1, | ||
| wed: 2, | ||
| thu: 3, | ||
| fri: 4, | ||
| sat: 5, | ||
| sun: 6, |
There was a problem hiding this comment.
This mapping sets mon: 0, tue: 1, etc., which makes Monday appear in the first column when applied via margin-left. The requirements say the calendar starts from Sunday by default and "Monday is the 1st, Friday is the 5th" relative to that. Adjust the offsets so that with Sunday as column 1, Monday shifts into the second column and other days align correctly.
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 are still two critical functional mismatches with the task requirements. First, your start-day logic currently treats Monday as column 0 and Sunday as column 6, but the description requires a Sunday-first calendar where Sunday has offset 0, Monday 1, …, Saturday 6; this is essential for “[Changing 'month-lengh' and 'start-day' modifier reflects in changing calendar layout]” and for the rule that “Monday is the 1st, Friday is the 5th” when counting from Sunday. To fix this, update your $week-days map (in src/styles/main.scss or wherever it is defined) so that the offsets are sun: 0, mon: 1, tue: 2, wed: 3, thu: 4, fri: 5, sat: 6, and ensure the calendar--start-day-* margin/offset calculations use those values. Second, the calendar must be centered relative to the full screen using the page element, but currently only .page__body is a flex container; you should add styles for html.page / .page (for example, display: flex; justify-content: center; align-items: center; min-height: 100vh;) so the calendar block is truly centered in the viewport as required. These two issues directly affect required behavior and checklist item #1, so they need to be addressed before the solution can be considered complete. Once you adjust the weekday offsets and use the page element to center the calendar, the rest of your implementation looks solid and should fully meet the task’s specifications.
✨ 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" | ||
| href="./styles/main.scss" | ||
| /> | ||
| </head> | ||
| <body> | ||
| <h1>Calendar</h1> | ||
| <!-- #endregion head --> | ||
| <body class="page__body"> | ||
| <div class="calendar calendar--start-day-sun calendar--month-length-31"> |
There was a problem hiding this comment.
This violates the description requirement that "Set calendar to start from Sunday by default" and that "The month should start at the correct column (Monday is the 1st, Friday is the 5th)" when counting from Sunday. Currently mon: 0 and sun: 6 make Monday the first column and Sunday the seventh. You should adjust $week-days so Sunday has offset 0, Monday 1, ..., Saturday 6, ensuring that calendar--start-day-* modifiers place days in the correct columns and satisfy checklist item #1 about changing start-day reflecting correct layout.
| flex-wrap: wrap; | ||
| width: $calendar-width; | ||
| box-sizing: border-box; | ||
| padding: $calendar-padding; | ||
| gap: $calendar-gap; | ||
|
|
||
| @for $length from 28 through 31 { | ||
| &--month-length-#{$length} { |
There was a problem hiding this comment.
This violates the requirement and checklist item #1: Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout because your $week-days map currently treats Monday as column 0 and Sunday as column 6, so the calendar layout does not start from Sunday as the first column as specified. You should update the offsets so sun: 0, mon: 1, ..., sat: 6, which will make the default calendar--start-day-sun place the first day in the first column and ensure “Monday is the 1st, Friday is the 5th” relative to a Sunday-first week.
| @@ -0,0 +1,22 @@ | |||
| $day-size: 100px; | |||
| $day-bg-color: #eee; | |||
| $day-border: 1px solid #000; | |||
There was a problem hiding this comment.
The task description requires the calendar to be centered in the middle of the screen both horizontally and vertically using the page element (class="page" on html), but currently only .page__body is a flex container; html.page itself is not styled, so the calendar may not be centered relative to the full viewport. Consider adding styles for .page/html.page to align the calendar in the center of the screen.
|
Please understand, I can't fulfill your condition "You should update the offsets so sun: 0, mon: 1, ..., sat: 6, which will make the default calendar--start-day-sun place the first day in the first column and ensure “Monday is the 1st, Friday is the 5th” relative to a Sunday-first week." |
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 as an educational override to help you progress while learning, even though there is still one semantic issue to fix. You did a very good job with the SCSS structure, BEM naming, flex layout, hover animation, month-length modifiers, and centering the calendar on the page—these parts match the requirements nicely.
The remaining problem is with the $week-days map in src/styles/utils/variables.scss (and where it’s reused): it currently treats Monday as offset 0 and Sunday as 6, which makes your calendar--start-day-* modifiers behave as a Monday-first grid and violates the Sunday-first semantics required in [CHECKLIST ITEM #1]. To align with the task, you should change that map to:
$week-days: (
sun: 0,
mon: 1,
tue: 2,
wed: 3,
thu: 4,
fri: 5,
sat: 6,
);while keeping your existing margin-left formula, so calendar--start-day-sun truly starts in the first (Sunday) column and other modifiers shift correctly. Overall, your implementation shows strong understanding of SCSS, layout, and modifiers; once you adjust the weekday offsets, your calendar will fully match the Sunday-first requirement. Keep going—this is a solid solution with just a small conceptual tweak left to master.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| <link | ||
| rel="stylesheet" | ||
| href="styles/index.scss" | ||
| href="./styles/main.scss" | ||
| /> | ||
| </head> | ||
| <body> | ||
| <h1>Calendar</h1> | ||
| <!-- #endregion head --> | ||
| <body class="page__body"> | ||
| <div class="calendar calendar--start-day-sun calendar--month-length-31"> |
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" together with the description rule "Set calendar to start from Sunday by default". Your $week-days map uses Monday-first offsets (mon: 0, ..., sun: 6), so the calendar--start-day-* modifiers position days as if Monday were column 0. You should change this map to Sunday-first (sun: 0, mon: 1, tue: 2, wed: 3, thu: 4, fri: 5, sat: 6) while keeping your existing margin-left calculation so the calendar responds correctly when start-day changes.
| gap: $calendar-gap; | ||
|
|
||
| @for $length from 28 through 31 { | ||
| &--month-length-#{$length} { |
There was a problem hiding this comment.
This still violates the Sunday-first requirement tied to checklist item #1: "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout" and the description statement "Set calendar to start from Sunday by default in your HTML". Your $week-days map treats Monday as offset 0 and Sunday as offset 6, which makes the calendar--start-day-sun modifier place the first day in the last column of a Monday-first grid. Update this map to sun: 0, mon: 1, tue: 2, wed: 3, thu: 4, fri: 5, sat: 6 while keeping your margin-left formula so the calendar is truly Sunday-first.
| $week-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" – your $week-days map uses Monday-first offsets (mon: 0, ..., sun: 6) instead of the required Sunday-first offsets (sun: 0, mon: 1, ..., sat: 6), so the calendar--start-day-* behavior does not match the task’s Sunday-first semantics where "Monday is the 1st, Friday is the 5th" relative to Sunday.
2 participants
