Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are a few critical issues that break required functionality and layout. First, in src/styles/utils/variables.scss, $border is defined as 1 without units, so rules like border: $border solid $color-black; compile to 1 solid #000, which is invalid and violates the 1px black border requirement; set $border: 1px; to fix this. Second, the $days map currently maps mon: 0, ... sun: 6, but your calendar grid uses Sunday as the first column, so calendar--start-day-mon incorrectly places Monday in the Sunday column; update the map to use Sunday as index 0, e.g. sun: 0, mon: 1, ... sat: 6, so start-day modifiers align correctly. Third, in src/styles/blocks/calendar.scss, the margin calculation for start-day modifiers uses $index * ($day-size + $gap), but the @each $day, $index loop is 1-based, so every start day is shifted one column too far; use ($index - 1) * ($day-size + $gap) so the first day can have zero margin when appropriate and the month starts in the correct column. Finally, the cursor requirement says the pointer should appear on hover, but you set cursor: pointer on .calendar__day globally; move cursor: pointer into the &:hover block so it only applies while hovering. Once these layout and validity issues are fixed, the rest of your implementation (numbers via ::before, modifiers, hover animation, and HTML structure) looks solid and 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
|
|
||
| @each $day, $index in $days { | ||
| &--start-day-#{$day} .calendar__day:first-child { | ||
| margin-left: $index * ($day-size + $gap); |
There was a problem hiding this comment.
This violates the requirement "The month should start at the correct column (Monday is the 1st, Friday is the 5th)." and the guidance "You can just add correct margin-left for the first day."
You use $index * ($day-size + $gap) but Sass @each $item, $index gives a 1-based index. That makes sun (index 1) produce a non-zero margin and shifts every start by one column. Use ($index - 1) * ($day-size + $gap) so the first day can have zero margin when appropriate.
src/styles/blocks/calendar.scss
Outdated
| background-color: $color-light-gray; | ||
| border: $border solid $color-black; | ||
| box-sizing: border-box; | ||
| cursor: pointer; |
There was a problem hiding this comment.
This violates the requirement "On hovering over a cell: cursor should become pointer."
You set cursor: pointer on .calendar__day unconditionally (line 22). The spec asks for the pointer to appear on hover. Move cursor: pointer into the &:hover block (line 38) so the pointer appears only while hovering. (Note: cursor changes are not animatable — that is fine.)
| $day-size: 100px; | ||
| $columns: 7; | ||
| $gap: 1px; | ||
| $border: 1px; |
There was a problem hiding this comment.
This violates the implementation detail "Each day is a grey (#eee) 100px square (including 1px black border)."
$border is declared as 1 without a unit. When used as border: $border solid $color-black; this will produce an invalid CSS value (e.g. 1 solid #000). Add the unit: $border: 1px; so the output becomes 1px solid #000.
| $days: ( | ||
| mon: 0, | ||
| tue: 1, | ||
| wed: 2, | ||
| thu: 3, | ||
| fri: 4, | ||
| sat: 5, | ||
| sun: 6 | ||
| ); |
There was a problem hiding this comment.
This breaks the requirement "The month should start at the correct column (Monday is the 1st, Friday is the 5th)."
The $days map currently maps mon: 0, tue: 1, ... sun: 6. Since the calendar layout uses Sunday as the first column, the map values must represent column indices starting from Sunday = 0. With the current mapping a --start-day-mon modifier will set margin-left to 0 columns (placing the 1st on Sunday) instead of Monday. Change the map to reflect column indices from Sunday, e.g.:
( sun: 0, mon: 1, tue: 2, wed: 3, thu: 4, fri: 5, sat: 6 ) so @each produces correct offsets.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because the start-day positioning logic is still incorrect, which breaks a core functional requirement of the calendar. The task requires that changing the calendar--start-day-* modifier shifts the first day into the correct weekday column (e.g., Monday in the 2nd column, Friday in the 6th), but with your current margin-left calculation on .calendar__day:first-child some start-day values place the first day in the wrong column. This means the checklist item that “changing start-day modifier reflects in changing calendar layout” is not fully satisfied and the calendar will render wrong for several configurations.
To fix this, make sure your margin-left formula in src/styles/blocks/calendar.scss uses the same indexing base as your $days map in src/styles/utils/variables.scss. Right now your $days map uses sun: 0 through sat: 6; adjust either (1) the map to be sun: 1 through sat: 7 and keep the current formula, or (2) keep the map as is and change the formula to use $index * ($day-size + $gap) (without - 1) so that Sunday (0) yields zero margin and other days offset correctly. After updating, verify manually that all modifiers from calendar--start-day-sun to calendar--start-day-sat produce the expected first-day column while month-length modifiers (28–31) still hide the correct extra days. Everything else — SCSS structure, hover effects, widths, variables, and HTML markup — looks solid and well-aligned with the requirements, so once this alignment bug is fixed your solution will fully meet the task expectations.
✨ 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
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as part of an educational override so you can continue progressing while you learn, because overall your calendar is very well done and matches the key requirements (BEM structure, SCSS organization, flex layout, modifiers, and hover effects). You correctly kept the HTML days unnumbered and generated numbers via ::before and an @for loop, and your use of variables and mixins makes the styles clean and maintainable. The start-day and month-length modifiers are implemented in SCSS so that changing calendar--start-day-* or calendar--month-length-* in the HTML updates the layout as required [CHECKLIST ITEM #1].
One area you could refine is the start-day positioning logic in your mixins file: double-check how the index from your $days map is used in the margin calculation so that each calendar--start-day-* modifier reliably puts day 1 into the correct weekday column; aligning the index base (0-based vs 1-based) with your margin formula is the key idea here. As a small robustness improvement, explicitly setting display (and possibly line-height) on .calendar__day::before can help ensure the number stays perfectly centered across different browsers. Overall, this is a strong implementation with good SCSS structure—fixing the start-day indexing detail will make it fully solid and closer to production-ready quality.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
DEMO LINK