add task solution#5445
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 few critical, functional mismatches with the task requirements that block completion. First, your styles are in src/styles/index.scss and the HTML links to styles/index.scss, but the task explicitly requires using src/styles/main.scss as the entry; you should move or rename index.scss to main.scss and update <link rel="stylesheet" href="styles/main.scss"> in src/index.html. Second, the month-length modifier names don’t align: HTML uses calendar--month-length--31 while SCSS generates &--length-#{$length}, so changing month-length in the class does not affect layout; adjust the loop to generate &--month-length--#{$length} so that modifiers like .calendar--month-length--28–31 correctly hide extra days [CHECKLIST ITEM #1]. Third, your start-day offset map (mon: 0, sun: 6, etc.) combined with the current margin-left calculation places day 1 under the wrong weekday columns; update the offsets so that calendar--start-day--sun starts in the Sunday column, --mon in the Monday column, and --fri in the fifth column, as the description requires [CHECKLIST ITEM #1]. Finally, the hover transition currently animates only transform, but the requirement says all hover changes (including background color) must be animated for 0.5s; set transition: transform 0.5s, background-color 0.5s; on the base .calendar__day so both properties are smoothly animated. You’ve done a solid job with BEM naming, flex layout, ::before numbering, and general SCSS structure—once these functional issues are fixed, your solution should meet the task requirements fully.
✨ 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--start-day--sun calendar--month-length--31"> |
There was a problem hiding this comment.
The calendar modifier for month length in HTML is calendar--month-length--31, but in SCSS you generate &--length-#{$length}. This mismatch means changing month-lengh in the HTML class will not affect layout, violating checklist item "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout". Align the SCSS modifier name with the HTML one (e.g. &--month-length--#{$length}).
| tue: 1, | ||
| wed: 2, | ||
| thu: 3, | ||
| fri: 4, |
There was a problem hiding this comment.
The modifier calendar--month-length--31 in HTML doesn’t match any SCSS modifier (&--length-#{$length}), so changing month-length in the code won’t affect layout. This violates checklist item #1: "Changing 'month-lengh' ... reflects in changing calendar layout"; align the modifier name in SCSS (e.g. &--month-length--#{$length}).
| $day-step: $day-size + $calendar-gap; | ||
| $weekdays: ( | ||
| mon: 0, |
There was a problem hiding this comment.
Task description requires writing styles in src/styles/main.scss, but this file links styles/index.scss. You need to point to styles/main.scss (or make main.scss the entry that includes your calendar styles).
| &:hover { | ||
| cursor: pointer; | ||
| background-color: #ffbfcb; | ||
| transform: translateY(-20px); | ||
| transition: transform 0.5s; |
There was a problem hiding this comment.
Hover transition currently animates only transform, but the requirement says all changes (including background color) must be animated with duration 0.5s. Extend transition to include background-color as well.
| @for $length from 28 through $calendar-days-count { | ||
| &--length-#{$length} { |
There was a problem hiding this comment.
This loop generates &--length-#{$length} modifiers (e.g. calendar--length-31), but HTML uses calendar--month-length--31. Because of this mismatch, the month-length modifier does not control layout as required (checklist item #1).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there is a critical mismatch between the modifier class names in your HTML and those generated in your SCSS, which breaks the requirement that changing start-day and month-length in the code must change the calendar layout [CHECKLIST ITEM #1]. In src/index.html you use modifiers like calendar--start-day-sun and calendar--month-length-31, but in src/styles/main.scss you generate both &--start-day--#{$day} / &--start-day-#{$day} and &--month-length--#{$length} / &--month-length-#{$length}, so only some of the styles ever apply and some selectors (like &:not(.calendar--start-day--sun)) never match your HTML. This inconsistency is a functional blocker, because it means that updating the modifiers on the .calendar element does not reliably move the first day or hide extra days as the task describes.
To fix this, choose one BEM-compatible naming convention and use it consistently for both HTML and SCSS, for example: .calendar--start-day-sun and .calendar--month-length-31 in HTML, and corresponding &--start-day-sun / &--month-length-31 in SCSS (or the double-dash variant, as long as both files match exactly). Then remove the duplicate @each and @for blocks that generate the unused modifier variants, so you have a single implementation of start-day and a single implementation of month-length. After that, verify that changing start-day (e.g. to calendar--start-day-wed) correctly shifts the first day’s column and that changing month-length (e.g. to calendar--month-length-29) hides the appropriate trailing days. Everything else in your layout, hover behavior, and use of SCSS features looks solid, so once you align the class names and clean up duplicates, your solution should fully meet the requirements. Keep going—you’re very close to a complete, correct implementation.
✨ 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
| <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 here use single dashes (calendar--start-day-sun, calendar--month-length-31), but your SCSS defines both &--start-day--#{$day}/&--start-day-#{$day} and &--month-length--#{$length}/&--month-length-#{$length}. This can cause the layout not to change correctly when you modify these classes, which violates checklist item #1: "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout". Align the modifier naming in SCSS to exactly match the HTML classes you use here and remove the unused variants.
| wed: 2, | ||
| thu: 3, | ||
| fri: 4, | ||
| sat: 5, |
There was a problem hiding this comment.
The calendar element uses calendar--start-day-sun and calendar--month-length-31, but in SCSS you generate modifiers as &--start-day--#{$day} and &--month-length--#{$length} (double dash before the value). This mismatch violates checklist item #1: "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout" because changing the HTML modifiers does not trigger the intended styles. Align the class naming convention between HTML and SCSS and keep only one consistent pattern.
| @each $day, $offset in $weekdays { | ||
| &--start-day-#{$day} { | ||
| .calendar__day:first-child { | ||
| margin-left: if($offset > 0, $offset * $day-step, 0); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This second @each block creates &--start-day-#{$day} modifiers, which are redundant and still do not match the HTML pattern (calendar--start-day-sun). Keeping two different modifier naming schemes is confusing and violates the requirement that changing start-day in the code element should affect layout [CHECKLIST ITEM #1]. Choose one correct BEM-compatible naming convention and remove the extra set.
| @for $length from 28 through $calendar-days-count { | ||
| &--month-length--#{$length} { | ||
| .calendar__day:nth-child(n + #{$length + 1}) { | ||
| display: none; | ||
| } | ||
| } |
There was a problem hiding this comment.
You generate &--month-length--#{$length} modifiers here, but the HTML uses calendar--month-length-31 (single dash before 31). Because of this mismatch, changing the month-length class in HTML does not hide days as intended, violating checklist item #1: "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout". Make the HTML class names and SCSS modifiers consistent and keep only one version.
| @for $length from 28 through $calendar-days-count { | ||
| &--month-length-#{$length} { | ||
| .calendar__day:nth-child(n + #{$length + 1}) { | ||
| display: none; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This second @for block creates &--month-length-#{$length} modifiers, which are redundant relative to the previous block and contribute to inconsistent naming. The task requires a single month-length modifier implementation using @for that directly controls which days are shown [CHECKLIST ITEM #1]. Remove the duplicate block and ensure the remaining one matches the HTML class naming.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: NCM]
If the issue is within your control, please address it.
The next review attempt will automatically launch in about 1 hour.
If you believe this is an error or need assistance, please contact the support team.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it fully meets the functional and technical requirements for the calendar component. You correctly used BEM naming (.calendar, .calendar__day, and block-only modifiers), SCSS variables for all repeated sizes and spacing, and generated both the day numbers and the month-length/start-day modifiers using @for and @each as required. The calendar layout behaves properly when changing calendar--start-day-* and calendar--month-length-* on the .calendar element, satisfying the dynamic layout expectations and [CHECKLIST ITEM #1].
The hover interaction is also well done: the cells change to a pink #FFBFCB background, move up with transform: translateY(-20px), and animate over 0.5s, which matches the specification. Your markup has exactly 31 .calendar__day elements with numbers injected via ::before, and the flex-based 7-column grid with gaps and padding is calculated cleanly via variables. Overall, this is a solid, clean implementation that shows good understanding of SCSS, BEM, and layout calculations; keep up this level of structure and clarity in future tasks.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
reflects in changing calendar layout
Typical MistakesfromBEMlesson theory are checked.