task#5440
Conversation
davidmonn
commented
Jun 22, 2026
- DEMO LINK
- TEST REPORT LINK
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because several core task requirements are not yet met, so the calendar behavior and layout are still incorrect. The HTML only has 30 .calendar__day elements and must have 31, and the default modifiers in markup need to represent a Sunday start and a 31‑day month (e.g. calendar calendar--sun calendar--month-length-31) as required. The calendar is not centered vertically and horizontally in the viewport; you should make body (or html, body) a full-height flex container with justify-content: center; align-items: center; instead of relying on padding-top. The start-day logic currently uses a ($weekValue * 6) factor, which produces a huge incorrect offset; it must shift the first day by 0–6 single day-widths computed from your size/gap variables, and the month-length modifiers (28–31) with @for + nth-child are completely missing, so changing month-length does not affect the layout [CHECKLIST ITEM #1].
Visually, each day is supposed to be a 100px square including the 1px border, but your current width: 100px plus border yields a larger box, and the container doesn’t restrict itself to exactly 7 columns with 1px gaps and 10px paddings or use $columns-days to compute that width. The day numbers added via ::before don’t yet enforce font-family: Arial and font-size: 30px, and the hover transform: translateY(-20px) uses a hardcoded 20px that should be extracted into a variable if reused; also, make the transition explicitly cover transform and background-color. Finally, there is a stylelint error in src/styles/index.scss where flex-direction and flex-wrap should be combined into the shorthand flex-flow. Once you address these functional and specification-level issues, your solution will align much better with the task, and the existing good parts (SCSS usage, @for for numbering, clean day markup) will really shine.
✨ 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
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <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.
Each day should be a grey 100px square including 1px border, and repeated pixel values should be replaced with variables. With width: $value-default where $value-default: 100px and border: 1px solid, the total size becomes 102px. To match the requirement and avoid repeated px values, adjust the variable or use box-sizing / derived variables so the final square including border is exactly 100px.
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> |
There was a problem hiding this comment.
You are using transition-duration and transition-timing-function but not specifying the properties to animate; to make both transform and background-color animate as required, use a transition shorthand or explicit transition-property instead of relying on defaults.
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> |
There was a problem hiding this comment.
The hover transform uses a hardcoded 20px value. The task says Don't use hardcoded px values if they are used several times and to use variables for clarity; consider introducing a variable (e.g. $hover-shift) and using it both in this transform and any related calculations instead of an inline literal.
| &:hover { | ||
| transform: translateY(-20px); |
There was a problem hiding this comment.
The hover transform uses a hardcoded 20px value. The description says "Don't use hardcoded px values if they are used several times" and encourages variables for clarity; consider extracting this into a variable (e.g. $hover-shift) if you use it elsewhere, and ensure alignment with that rule.
| @for $numbers from 1 through 31 { | ||
| .calendar__day:nth-child(#{$numbers})::before { | ||
| content: '#{$numbers}'; |
There was a problem hiding this comment.
The day numbers are added via ::before, but you are not setting font-family: Arial and font-size: 30px on this pseudo-element as required: "Add a number (Arial 30px) in the center of each day using ::before and @for."
| @each $dayWeek, $weekValue in $daysWeek { | ||
| .calendar--#{$dayWeek} { | ||
| .calendar__day:first-child { | ||
| margin-left: ($value-default + $distance-box) * ($weekValue * 6); |
There was a problem hiding this comment.
The margin-left calculation multiplies by ($weekValue * 6), which makes the offset far too large and does not correspond to a 0–6 column shift. Also, it ignores $columns-days and padding. This breaks checklist item "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout" for start-day, and does not satisfy "The month should start at the correct column".
| // #region ADD DAYS | ||
| @for $numbers from 1 through 31 { | ||
| .calendar__day:nth-child(#{$numbers})::before { | ||
| content: '#{$numbers}'; | ||
| } | ||
| } | ||
|
|
||
| // #endregion | ||
|
|
||
| // #region Start Day | ||
| $daysWeek: ( | ||
| sun: 0, | ||
| mon: 1, | ||
| tue: 2, | ||
| wed: 3, | ||
| thu: 4, | ||
| fri: 5, | ||
| sat: 6, | ||
| ); | ||
|
|
||
| @each $dayWeek, $weekValue in $daysWeek { | ||
| .calendar--#{$dayWeek} { | ||
| .calendar__day:first-child { | ||
| margin-left: ($value-default + $distance-box) * ($weekValue * 6); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // #endregion |
There was a problem hiding this comment.
There is no implementation of the month-length modifier (calendar--month-length--28, --29, --30, --31) using @for and nth-child to control the last visible day, even though you use calendar--month-length--31 in HTML. This violates the requirement: "Add a modifier month-length for the calendar with values 28, 29, 30 and 31 (use @for). It sets the last day to show (use nth-child)."
| .calendar { | ||
| display: flex; | ||
| margin: auto; | ||
| } | ||
|
|
There was a problem hiding this comment.
You define $columns-days here but never use it to compute the calendar width or start-day offsets, missing the requirement to "Use flex with 1px gap and limit its width to exactly 7 columns + 10px paddings" and to "Use properly named variables to make all the calculations more clear."
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because several core requirements from the task are still not met and they block correct calendar behavior. The calendar root element does not use the required default modifiers (calendar calendar--sun calendar--month-length-31), the HTML contains only 30 .calendar__day elements instead of 31, and there is no implemented month-length modifier logic (calendar--month-length-28…31 with @for/nth-child), so changing month length cannot affect the layout [CHECKLIST ITEM #1]. The start-day offset formula is incorrect (it multiplies by 6 and uses a magic + 2), so the first day is not shifted by 0–6 single day widths as required, and currently changing start-day will not correctly move the calendar grid [CHECKLIST ITEM #1]. Each day is specified to be a 100px square including the 1px border, but you set width: 100px; height: 100px; border: 1px, which yields about 102×102px; this needs to be adjusted (e.g. via box-sizing or smaller inner size) so the outer box is exactly 100×100. The calendar container also must be a flex layout limited to exactly 7 columns with 1px gaps and 10px paddings, computed using your variables (including $columns-days), but right now the container width and paddings are not calculated at all, and the calendar is not centered vertically in the viewport using full-height flex on html, body. Finally, the hover transition is declared as transition: 0.5s linear; without explicitly listing transform and background-color, which was a specific requirement for the hover animation.
To fix this, please: 1) update the HTML root class to calendar calendar--sun calendar--month-length-31 and ensure there are 31 .calendar__day divs, 2) implement calendar--month-length-28/29/30/31 in SCSS with @for and nth-child to hide extra days, 3) correct the start-day logic so the first visible day gets margin-left: singleDayWidth * weekValue using your variables (no extra * 6 or + 2), 4) adjust day size so the total including border is exactly 100px, 5) compute .calendar__container width and paddings for exactly 7 columns using $columns-days, $distance-box, and your size variables, and 6) center the calendar using html, body { height: 100%; display: flex; justify-content: center; align-items: center; } and explicitly transition transform and background-color on hover. You’ve done a good job with SCSS, BEM elements, and generating numbers via @for, so once these functional and layout issues are addressed your solution will be very close to the specification.
✨ 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
| </head> | ||
| <body> | ||
| <h1>Calendar</h1> | ||
| <div class="calendar calendar--mon calendar--start_day--31"> |
There was a problem hiding this comment.
The calendar element must start from Sunday by default and use the required modifiers. Current class="calendar calendar--mon calendar--start_day--31" violates both: 1) it should use calendar--sun by default, and 2) the month-length modifier name must follow the month-length pattern, e.g. calendar--month-length-31. This contradicts the description and checklist item #1 about start-day and month-lengh modifiers reflecting the layout.
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <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.
There must be 31 .calendar__day elements in the markup, but currently there are only 30. This breaks the requirement: "Create a markup for the calendar block with 31 days inside" and the previous review correction that HTML must have 31 .calendar__day elements.
| <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.
Each day must be a 100px square including the 1px border, but here width and height are set to 100px while the border is added separately, yielding a 102×102 box. You need to adjust the dimensions (or use box-sizing) so that the overall size including border is 100×100 as specified.
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> |
There was a problem hiding this comment.
The transition is declared as just 0.5s linear, which applies to all animatable properties. The requirements and previous feedback call for explicitly transitioning transform and background-color with 0.5s duration, so consider specifying those properties in the transition shorthand.
| margin: auto; | ||
|
|
||
| flex-flow: row wrap; | ||
|
|
There was a problem hiding this comment.
The calendar should start from Sunday and use a month-length modifier as specified, for example: class="calendar calendar--sun calendar--month-length-31". The current calendar--mon and calendar--start_day--31 modifiers don't match the required naming or defaults, so changing start-day/month-length in the code element cannot correctly update the layout (violates checklist item: "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout").
| &:last-child { | ||
| justify-content: flex-start; | ||
| } | ||
|
|
There was a problem hiding this comment.
The body layout uses flex-flow: row wrap which is unnecessary here and doesn’t center the calendar vertically; per the requirements the main goal is centering the calendar in the viewport, so you should focus on justify-content/align-items with full height rather than wrapping.
| width: $value-default; | ||
| height: $value-default; |
There was a problem hiding this comment.
The day size is set to width: $value-default; height: $value-default; with $value-default: 100px while also adding border: 1px solid black;. The specification says each day is a 100px square including the 1px border, so this currently overshoots and needs adjustment so the outer size is exactly 100px.
| .calendar__container { | ||
| display: flex; | ||
| gap: $distance-box; | ||
| flex-flow: row wrap; | ||
| justify-content: center; |
There was a problem hiding this comment.
The container currently has no width or padding based on $columns-days and does not enforce "flex with 1px gap and limit its width to exactly 7 columns + 10px paddings". You define $columns-days in var.scss but never use it, so the container sizing requirement from the description is not yet satisfied.
| cursor: pointer; | ||
| transition: 0.5s linear; |
There was a problem hiding this comment.
The hover transition is declared as a generic transition: 0.5s linear;. The previous corrections asked to explicitly transition the properties you change (at least transform and background-color) so it’s clear and matches the spec about animating those changes for 0.5s.
| @each $dayWeek, $weekValue in $daysWeek { | ||
| .calendar--#{$dayWeek} { | ||
| .calendar__day:first-child { | ||
| margin-left: ($value-default + $distance-box + 2) * ($weekValue * 6); |
There was a problem hiding this comment.
The start-day offset calculation margin-left: ($value-default + $distance-box + 2) * ($weekValue * 6); is incorrect: it uses a magic + 2 and multiplies the index by 6, causing a huge shift. According to the task, the first day should be shifted by 0–6 single day-widths (including gap/border) computed from your variables, so the formula needs to be reworked to use $weekValue directly (without the extra * 6) and to rely only on meaningful variables rather than a hardcoded 2. This also means changing the start-day modifier should visibly adjust the calendar layout (checklist item "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout").
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 a few important points to improve next. You used SCSS, BEM naming, variables, and @for/@each well, and the days are correctly styled as 100×100 squares with hover movement and color change. The main blocking issues are: the root element must use class="calendar calendar--sun calendar--month-length-31" (instead of calendar--mon / calendar--start_day--31), the HTML must contain 31 .calendar__day elements, and you still need to implement calendar--month-length-28…31 using @for + nth-child so changing month-length actually hides extra days [CHECKLIST ITEM #1]. Also, the start-day offset formula should shift the first day by 0–6 single-day widths (remove the * 6 factor in your margin-left math), and the calendar should be centered vertically by using a full-height flex layout on html, body instead of a fixed body width and top padding. For the container, compute .calendar__container width and paddings from your variables so it fits exactly 7 columns with 1px gaps and 10px side paddings, and update the hover rule to transition: transform 0.5s linear, background-color 0.5s linear; as required. Overall, your structure is solid and you are close—fixing the modifier naming/logic, layout math, and centering will take your solution to a much more polished, spec-compliant level.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| </head> | ||
| <body> | ||
| <h1>Calendar</h1> | ||
| <div class="calendar calendar--mon calendar--start_day--31"> |
There was a problem hiding this comment.
The calendar root element must use the required default modifiers calendar calendar--sun calendar--month-length-31; currently it uses calendar calendar--mon calendar--start_day--31, which also misnames the month-length modifier, violating the requirement and checklist item #1 about changing 'month-lengh' affecting the layout.
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <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.
HTML must contain 31 .calendar__day elements by default, but there are only 30 here, so the markup does not meet the "Create a markup for the calendar block with 31 days inside" requirement.
| <div class="calendar__container"> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <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.
Each day must be exactly a 100px square including the 1px border; with width: 100px; height: 100px; border: 1px you need either box-sizing: border-box applied correctly or reduced inner size so the outer size is 100×100, currently this requirement is not clearly satisfied.
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> |
There was a problem hiding this comment.
For hover animation, you should explicitly list the properties being transitioned (e.g. transform and background-color) instead of transition: 0.5s linear;, as required by the task and previous corrections.
| margin: auto; | ||
|
|
||
| flex-flow: row wrap; | ||
|
|
There was a problem hiding this comment.
The root calendar element should use the required default modifiers calendar calendar--sun calendar--month-length-31 (start from Sunday, 31 days) instead of calendar calendar--mon calendar--start_day--31. Also, there is no calendar--month-length-… modifier implemented in your SCSS, which violates the description and checklist item about changing month-length and start-day affecting layout.
| cursor: pointer; | ||
| transition: 0.5s linear; |
There was a problem hiding this comment.
The transition does not explicitly specify properties, which conflicts with the requirement (and previous review note) to animate hover changes with a 0.5s duration while listing properties like transform and background-color explicitly.
| @each $dayWeek, $weekValue in $daysWeek { | ||
| .calendar--#{$dayWeek} { | ||
| .calendar__day:first-child { | ||
| margin-left: ($value-default + $distance-box) * ($weekValue * 6); |
There was a problem hiding this comment.
Checklist item #1 requires that changing start-day and month-length modifiers affects layout, but the start-day offset math is incorrect: multiplying by ($weekValue * 6) over-shifts the first day. It should be based on $weekValue single-day widths (using your variables) without the extra * 6.
| @for $numbers from 1 through 31 { | ||
| .calendar__day:nth-child(#{$numbers})::before { | ||
| content: '#{$numbers}'; | ||
| font-family: Arial, Helvetica, sans-serif; | ||
| font-size: $font-size-default; | ||
| } | ||
| } | ||
|
|
||
| // #endregion | ||
|
|
||
| // #region Start Day | ||
| $daysWeek: ( | ||
| sun: 0, | ||
| mon: 1, | ||
| tue: 2, | ||
| wed: 3, | ||
| thu: 4, | ||
| fri: 5, | ||
| sat: 6, | ||
| ); | ||
|
|
||
| @each $dayWeek, $weekValue in $daysWeek { | ||
| .calendar--#{$dayWeek} { | ||
| .calendar__day:first-child { | ||
| margin-left: ($value-default + $distance-box) * ($weekValue * 6); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // #endregion |
There was a problem hiding this comment.
There is no implementation of the month-length modifier (calendar--month-length-28…31 using @for and nth-child) that hides extra days. This violates the description and checklist item #1 requirement that changing the month-length modifier changes the calendar layout.
| .calendar__container { | ||
| display: flex; | ||
| gap: $distance-box; | ||
| flex-flow: row wrap; | ||
| justify-content: center; | ||
|
|
||
| &:last-child { | ||
| justify-content: flex-start; | ||
| } | ||
| } |
There was a problem hiding this comment.
.calendar__container is missing the calculated width and 10px paddings for exactly 7 columns with 1px gaps, computed via your variables (e.g. $columns-days, $distance-box), which is required by the task; instead you rely on default width and set a hardcoded width on body.
| $value-default: 100px; | ||
| $distance-box: 1px; | ||
| $columns-days: 7; |
There was a problem hiding this comment.
The calendar itself should be centered using a full-height flex container on html, body with justify-content and align-items both set to center. Currently .calendar only has margin: auto; and body uses fixed width/padding, which does not satisfy the vertical centering requirement.