add markup#5446
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because several core requirements are still missing or wired incorrectly, which blocks the task from being considered complete. The calendar is not centered vertically and horizontally in the viewport, and the width/padding calculation (7 columns + 10px paddings) is wrong and doesn’t use the correct variables. Day numbers are not generated on .calendar__day::before with a proper @for loop and nth-child, so the calendar days remain unlabeled, and your existing loop targets a non‑existent .calendar__column. The start-day and month-length modifiers are present only as class names; there is no @each / @for + nth-child logic in SCSS to offset the first day or hide extra days, so changing these modifiers does not affect layout ([CHECKLIST ITEM #1]). Hover behavior on cells (pointer cursor, #FFBFCB background, transform: translateY(-20px), transition: 0.5s) is also completely missing.
To fix this, you should: (1) center the calendar using flexbox on body or a wrapper, (2) compute .calendar width from variables for day size, columns, gap, and padding, (3) generate numbers 1–31 with @for on .calendar__day:nth-child(#{$i})::before, (4) implement block-level modifiers like calendar--start-day-sun and calendar--month-length-31 with @each/@for to control first-day offset and visible days, and (5) add the specified hover styles and transitions on .calendar__day. Once these critical pieces are in place, the calendar will respond correctly to modifier changes and meet the checklist and visual behavior 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
| <link | ||
| rel="stylesheet" | ||
| href="styles/index.scss" | ||
| href="styles/main.scss" |
There was a problem hiding this comment.
width: $colums * $day-size + 6 * $gap; does not implement the spec: it doesn’t include the 10px paddings on both sides and $colums is a misspelling; revise this to use correctly named variables and include padding in the width calculation per the task requirements.
| href="styles/index.scss" | ||
| href="styles/main.scss" | ||
| /> | ||
| </head> |
There was a problem hiding this comment.
.calendar__column is defined here but never used in your HTML; this extra element complicates the BEM structure and is unnecessary for the required layout of 31 .calendar__day cells inside .calendar. Consider removing or reworking it to match the markup.
| <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 define the basic dimensions and styles of the day cell, but you don’t add any hover effects (pointer cursor, pink #FFBFCB background, 20px upward transform with 0.5s animation) that the task specifies.
| <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.
The @for loop is nested under &__day and targets &__column:nth-child(-n + #{$i})::before, but there is no .calendar__column in your HTML; this means day numbers are never generated. You should instead target .calendar__day:nth-child(#{$i})::before to render numbers 1–31 using ::before as required.
| </head> | ||
| <body> | ||
| <h1>Calendar</h1> | ||
| <div class="calendar calendar__start-day--sun calendar__month-length--31"> |
There was a problem hiding this comment.
The calendar modifiers calendar__start-day--sun and calendar__month-length--31 used here are never implemented in SCSS, so changing them won’t change the layout. This violates checklist item #1: Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout. You need @each for all start-day values and @for/nth-child to support all month-length values.
| gap: $gap; | ||
| flex-wrap: wrap; | ||
|
|
||
| width: $colums * $day-size + 6 * $gap; |
There was a problem hiding this comment.
The width calculation should match 7 columns + 10px paddings using variables. You hardcode 6 * $gap, but don’t include padding in the calculation here, and the variable $colums is misspelled. Make the formula clearly derive from the number of columns, gap, and padding variables as required.
| @for $i from 1 through 31 { | ||
| &__column:nth-child(-n + #{$i}) { |
There was a problem hiding this comment.
&__column refers to .calendar__day__column, which does not exist anywhere; also the requirement is to add numbers in the center of each day using ::before on the day itself. This @for loop will not generate any content and does not follow the described pattern. You should target .calendar__day:nth-child(#{$i})::before (or equivalent) instead.
| .calendar{ | ||
| display: flex; | ||
| gap: $gap; | ||
| flex-wrap: wrap; | ||
|
|
||
| width: $colums * $day-size + 6 * $gap; | ||
|
|
||
| &__column{ | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: $gap; | ||
| } | ||
|
|
||
| &__day{ | ||
| height: $day-size; | ||
| width: $day-size; | ||
|
|
||
| padding: $day-padding; | ||
|
|
||
| box-sizing: border-box; | ||
| border: $border; | ||
| background-color: $day-background-color; | ||
|
|
||
| @for $i from 1 through 31 { | ||
| &__column:nth-child(-n + #{$i}) { | ||
| &::before{ | ||
| content: #{$i}; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
You haven’t implemented any styles for start-day and month-length modifiers, nor used @each or nth-child as required. Add @each to generate .calendar--start-day-... modifiers that adjust the first day’s offset, and @for plus nth-child to control visible days for the month-length modifier (Checklist item #1).
| &__day{ | ||
| height: $day-size; | ||
| width: $day-size; | ||
|
|
||
| padding: $day-padding; | ||
|
|
||
| box-sizing: border-box; | ||
| border: $border; | ||
| background-color: $day-background-color; | ||
|
|
||
| @for $i from 1 through 31 { | ||
| &__column:nth-child(-n + #{$i}) { | ||
| &::before{ | ||
| content: #{$i}; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
On hover over a cell, the task requires pointer cursor, pink background #FFBFCB, moving the cell up by 20px with transform, and animating these changes over 0.5s. There is no :hover styling or transition on .calendar__day yet, so this requirement is missing.
| @@ -0,0 +1,6 @@ | |||
| $gap: 1px; | |||
| $border: 1px solid black; | |||
| $day-background-color: #eee; | |||
There was a problem hiding this comment.
The calendar container is not centered horizontally and vertically in the viewport as required; you need additional layout styles (e.g. on body or a wrapper) to satisfy the “Display a calendar in the middle of the screen” requirement.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is still a critical mismatch with the layout requirement for the calendar’s width. The task explicitly says the calendar width must be “exactly 7 columns + 10px paddings” and that these calculations should use variables. Right now, your width formula (e.g. width: $colums * $day-size + 6 * $gap; in src/styles/main.scss) only includes the 7 day columns and 6 gaps, and it does not account for the horizontal paddings ($day-padding / 10px) on both sides, so the resulting width does not match the spec.
To fix this, update the width calculation to include both left and right paddings via variables, something in the spirit of width: $columns * $day-size + (7 - 1) * $gap + 2 * $day-padding; (adjust naming to your actual variables). Make sure $day-padding (or equivalent) is defined in variables.scss and used instead of hardcoded 10px, so the total width is driven entirely by variables. As a minor improvement, consider correcting the typo $colums to $columns for clarity, but this is not blocking. Overall, the rest of the functionality and BEM structure look solid; once the width formula matches the “7 columns + 10px paddings” requirement, your solution will be ready to pass.
✨ 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 calendar--start-day-sun calendar--month-length-31"> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> |
There was a problem hiding this comment.
The requirement says "limit its width to exactly 7 columns + 10px paddings" and to use variables for these calculations. Here your width: $colums * $day-size + 6 * $gap; only accounts for 6 gaps and ignores $day-padding, so the resulting width does not match "7 columns + 10px paddings". Consider including both horizontal paddings and the correct number of gaps via variables so the total width is computed exactly as specified.
There was a problem hiding this comment.
i use additional paddings and do not add this padings to width
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 refine the layout details.
You did a strong job on the core calendar functionality: 31 days in the markup, BEM naming, SCSS structure, hover behavior, and the use of Sass loops and variables all align well with the task requirements. The main remaining issue is the calendar width calculation in src/styles/main.scss (around the width: declaration for .calendar): it currently uses width: $columns * $day-size + calc(7 - 1) * $gap; and does not add 2 * $day-padding, so the total width does not yet match the spec of 7 columns + 10px paddings on both sides [CHECKLIST ITEM #1]. This matters because the requirement explicitly says the paddings must be included in the width and handled via variables instead of hardcoded px values.
To fix this, update the width formula to include both left and right paddings via your $day-padding variable (conceptually: 7 * day-size + 6 * gap + 2 * day-padding), making sure the whole expression is driven by variables and not repeated literals. Once you adjust that, your layout will fully match the specification and your otherwise clean, well-structured implementation will be fully compliant. Keep up the solid work with SCSS and BEM—you’re very close to a perfect solution.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| <div class="calendar calendar--start-day-sun calendar--month-length-31"> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> |
There was a problem hiding this comment.
The calendar width is currently computed as width: $columns * $day-size + calc(7 - 1) * $gap;, which only includes the day columns and 6 gaps. The description requires the width to be "exactly 7 columns + 10px paddings", and variables must be used for repeated px values. This violates that requirement because the horizontal paddings ($day-padding on left and right) are not included in the width calculation. Consider updating the formula to add both left and right padding (via $day-padding) so the total width becomes 7 columns + 6 gaps + 2 * padding.
This is the same issue noted in the previous review and remains blocking until fixed.
|
|
||
| padding: $day-padding; | ||
|
|
||
| width: $columns * $day-size + calc(7 - 1) * $gap; |
There was a problem hiding this comment.
This violates the previous high-priority requirement to "update the calendar width formula so it includes horizontal paddings and matches the requirement ‘width to exactly 7 columns + 10px paddings’; use variables instead of hardcoded 10px". Your current width: $columns * $day-size + calc(7 - 1) * $gap; does not include 2 * $day-padding, so the total width omits the left and right paddings even though $day-padding is defined and used elsewhere.
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 ❗️