Conversation
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 one core functional requirement of the task is still not met: changing the start-day modifier does not correctly shift the first day of the month to the proper weekday column. According to the checklist, updating classes like calendar--start-day-mon or calendar--start-day-fri must visibly move the first .calendar__day into the corresponding weekday column, but your current $days-list map in src/styles/main.scss uses "mon": 0 … "sun": 6, which breaks this behavior. To fix this, adjust the map so that Sunday is the zero-offset and the rest follow sequentially, for example: "sun": 0, "mon": 1, "tue": 2, "wed": 3, "thu": 4, "fri": 5, "sat": 6, so that margin-left: ($day-size + $gap) * $multiplier places the first day in the correct column. This directly addresses the checklist item that says changing start-day must change the calendar layout, and it’s a functional, not cosmetic, issue.
Aside from that mapping bug, the rest of your implementation is strong: you correctly generate 31 days with SCSS, use ::before for numbers, calculate widths for 7 columns, implement month-length modifiers, and add the required hover animations. As a minor (non-blocking) improvement, you could limit transition to specific properties like background-color and transform instead of all for better performance, and consider adding keyboard focus styles parallel to :hover for accessibility. Once you correct the $days-list offsets so that each calendar--start-day-* class produces the expected visual shift, your solution will fully satisfy the functional requirements for this task.
✨ 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
| $days-list: ( | ||
| "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'. The $days-list map is incorrect — currently it maps "mon": 0 … "sun": 6. To position the first day at the correct weekday column (with Sunday as the default start), update the map so the multipliers are: "sun": 0, "mon": 1, "tue": 2, "wed": 3, "thu": 4, "fri": 5, "sat": 6. This will make margin-left: ($day-size + $gap) * $multiplier place the first .calendar__day in the correct column.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your progress on implementing the start-day and month-length modifiers is excellent, and the calendar generation logic is robust! However, changes are needed as the calendar is not currently centered on the screen, which is a core functional requirement and is causing visual regression tests to fail. To fix this, please add centering styles to the body element in src/styles/main.scss:
body {
margin: 0;
min-height: 100vh;
display: flex;
align-items: center;
justify-content: center;
}Additionally, please address the style linting warnings regarding CSS property order and empty line usage within the &__day selector for better code consistency. Once these adjustments are made, your solution should fully meet the task 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
| .calendar { | ||
| $days-list: ( | ||
| "sun": 0, | ||
| "mon": 1, | ||
| "tue": 2, | ||
| "wed": 3, | ||
| "thu": 4, | ||
| "fri": 5, | ||
| "sat": 6 | ||
| ); | ||
|
|
||
| display: flex; | ||
| flex-wrap: wrap; | ||
| gap: $gap; | ||
|
|
||
| box-sizing: border-box; | ||
| width: ($day-size * $columns) + ($gap * ($columns - 1)) + ($padding * 2); | ||
| padding: $padding; | ||
|
|
||
| &__day { | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| cursor: pointer; | ||
|
|
||
| box-sizing: border-box; | ||
| width: $day-size; | ||
| height: $day-size; | ||
|
|
||
| border: 1px solid $border-color; | ||
| background-color: $day-bg; | ||
|
|
||
| font-family: $font-family; | ||
| font-size: $font-size; | ||
|
|
||
| transition: | ||
| background-color $transition-speed, | ||
| transform $transition-speed; | ||
|
|
||
| &:hover { | ||
| transform: translateY(-20px); | ||
| background-color: $day-hover-bg; | ||
| } | ||
|
|
||
| @for $i from 1 through 31 { | ||
| &:nth-child(#{$i})::before { | ||
| content: "#{$i}"; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @each $name, $multiplier in $days-list { | ||
| &--start-day-#{$name} &__day:first-child { | ||
| margin-left: ($day-size + $gap) * $multiplier; | ||
| } | ||
| } | ||
|
|
||
| @for $limit from 28 through 31 { | ||
| &--month-length-#{$limit} { | ||
| .calendar__day:nth-child(n + #{$limit + 1}) { | ||
| display: none; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The task requires the calendar to be displayed in the middle of the screen (both horizontally and vertically). This typically involves styling the parent container (like page-container or body) with flexbox properties such as display: flex, align-items: center, justify-content: center, and ensuring it takes up full viewport height with min-height: 100vh. These centering styles are currently missing.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override to help you progress, even though there are a few areas for improvement. You've done excellent work implementing the start-day and month-length modifiers, and your overall SCSS structure and HTML markup are well-organized. The most critical next step is to address the calendar not being centered on the screen; please add the recommended body styles to src/styles/main.scss to fix this, as it's causing visual regression failures. Additionally, please review the property order and remove empty lines within the &__day selector in src/styles/main.scss to align with [CHECKLIST ITEM #4] and improve code consistency. Keep up the great work, and focus on these styling refinements for an even stronger solution!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| $day-size: 100px; | ||
| $day-bg: #eee; | ||
| $day-hover-bg: #ffbfcb; | ||
| $border-color: #000; | ||
| $gap: 1px; | ||
| $padding: 10px; | ||
| $columns: 7; | ||
| $transition-speed: 0.5s; | ||
| $font-family: Arial, sans-serif; | ||
| $font-size: 30px; | ||
|
|
||
| .calendar { | ||
| $days-list: ( | ||
| "sun": 0, | ||
| "mon": 1, | ||
| "tue": 2, | ||
| "wed": 3, | ||
| "thu": 4, | ||
| "fri": 5, | ||
| "sat": 6 | ||
| ); | ||
|
|
||
| display: flex; | ||
| flex-wrap: wrap; | ||
| gap: $gap; | ||
|
|
||
| box-sizing: border-box; | ||
| width: ($day-size * $columns) + ($gap * ($columns - 1)) + ($padding * 2); | ||
| padding: $padding; | ||
|
|
||
| &__day { | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| cursor: pointer; | ||
|
|
||
| box-sizing: border-box; | ||
| width: $day-size; | ||
| height: $day-size; | ||
| border: 1px solid $border-color; | ||
|
|
||
| background-color: $day-bg; | ||
| font-family: $font-family; | ||
| font-size: $font-size; | ||
|
|
||
| transition: | ||
| background-color $transition-speed, | ||
| transform $transition-speed; | ||
|
|
||
| &:hover { | ||
| transform: translateY(-20px); | ||
| background-color: $day-hover-bg; | ||
| } | ||
|
|
||
| @for $i from 1 through 31 { | ||
| &:nth-child(#{$i})::before { | ||
| content: "#{$i}"; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @each $name, $multiplier in $days-list { | ||
| &--start-day-#{$name} &__day:first-child { | ||
| margin-left: ($day-size + $gap) * $multiplier; | ||
| } | ||
| } | ||
|
|
||
| @for $limit from 28 through 31 { | ||
| &--month-length-#{$limit} { | ||
| .calendar__day:nth-child(n + #{$limit + 1}) { | ||
| display: none; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The previous review highlighted that the calendar is not centered on the screen and provided specific body styles to fix this. These styles are still missing from src/styles/main.scss. Please add the recommended body styles to ensure the calendar is properly centered both horizontally and vertically, as required by the task description.
| &__day { | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| cursor: pointer; | ||
|
|
||
| box-sizing: border-box; | ||
| width: $day-size; | ||
| height: $day-size; | ||
| border: 1px solid $border-color; | ||
|
|
||
| background-color: $day-bg; | ||
| font-family: $font-family; | ||
| font-size: $font-size; | ||
|
|
||
| transition: | ||
| background-color $transition-speed, | ||
| transform $transition-speed; | ||
|
|
||
| &:hover { | ||
| transform: translateY(-20px); | ||
| background-color: $day-hover-bg; | ||
| } | ||
|
|
||
| @for $i from 1 through 31 { | ||
| &:nth-child(#{$i})::before { | ||
| content: "#{$i}"; | ||
| } | ||
| } |
There was a problem hiding this comment.
To improve code consistency and adhere to general CSS property ordering conventions (part of [CHECKLIST ITEM #4]), consider grouping related properties. For example, layout properties (display, align-items, justify-content) usually come first, followed by box model (box-sizing, width, height, border), then visual (background-color, font-family, font-size), and finally interactive/transition properties (cursor, transition). Currently, cursor is placed earlier than typical.
| align-items: center; | ||
| justify-content: center; | ||
| cursor: pointer; | ||
|
|
There was a problem hiding this comment.
The previous review mentioned linting warnings regarding empty line usage within the &__day selector (part of [CHECKLIST ITEM #4]). There are still empty lines present, such as here and on lines 41, 45, 49, and 54. Please remove these empty lines for better code consistency.
2 participants
No description provided.