add task solution#5458
Conversation
fedorova-daryna
commented
Jun 30, 2026
fedorova-daryna
commented
- DEMO LINK
- TEST REPORT LINK
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 several core requirements of the task are not yet met, and they affect the calendar’s functionality and the checklist conditions. The calendar sizing is built with repeated hardcoded values like 100px and 1px (e.g. in src/styles/main.scss around the width and day sizes) and does not include the required 10px paddings, which breaks the “7 columns + paddings” rule and the “no repeated px without variables” requirement. The calendar--start-day-* modifiers are only implemented for sun and wed in both src/index.html and src/styles/main.scss, and they are written manually; you must use @each to generate all seven variants (mon–sun) and fix the Sunday logic so that a Sunday-start month places the first day in the first column instead of shifting it by 6 columns. Similarly, the month-length modifiers (28–31) are not generated with @for, and the calendar--month-length-31 rule is missing in SCSS, so changing month length in HTML will not correctly hide extra days as required by [CHECKLIST ITEM #1].
You also center the day numbers by making .calendar__day a flex container and not by styling the ::before pseudo-element itself; the task explicitly asked to add and center the numbers “using ::before and @for”, so it would be better to ensure the pseudo-element is the main content and is what gets centered. Finally, the hover animation only sets transition-duration and not a full transition (background and transform), which makes the intent less clear for the required “animate all changes with duration 0.5s”. To move forward, please: 1) introduce SCSS variables for day size, border width, gap, and paddings and reuse them in all calculations; 2) implement @each for all start-day modifiers with correct column offsets, ensuring Sunday has no extra margin; 3) implement @for for calendar--month-length-28..31 to hide days via nth-child; 4) adjust centering so numbers are controlled via ::before; and 5) define a clear transition for the hover properties. Once these changes are in place, your calendar will meet the functional and checklist requirements, and the solution will be ready to approve.
✨ 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
| rel="stylesheet" | ||
| href="styles/index.scss" | ||
| href="./styles/main.scss" | ||
| /> |
There was a problem hiding this comment.
calendar width is hardcoded with 100px and 1px repeated, and there is no padding included; this violates the requirement to “use properly named variables” and to limit width to “7 columns + 10px paddings” without duplicating px values.
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> |
There was a problem hiding this comment.
calendar__day uses hardcoded 100px sizes instead of SCSS variables reused in all calculations; this goes against the requirement to avoid repeated hardcoded px values.
| <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 only set transition-duration without specifying which properties should be animated; the requirement says “all changes should be animated with duration 0.5s”, so consider using a full transition definition for background and transform.
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> |
There was a problem hiding this comment.
The calendar--start-day-sun modifier shifts the first day by 6 columns, which makes Sunday appear in the last column; for a Sunday-start month, the first day should be in the first column and not shifted, so this doesn’t satisfy the start-day semantics.
| <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.
Only sun and wed variants are defined for calendar--start-day-*, and they are written manually; the task requires using @each to generate modifiers for mon, tue, wed, thu, fri, sat, and sun.
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| </div> |
There was a problem hiding this comment.
The loop for numbering days with ::before is correct, but the requirement was to add the number in the center of each day via ::before; currently the centering is done by flex on .calendar__day, not using any layout on the pseudo-element itself. Consider ensuring the pseudo-element content is what’s centered.
| .calendar--start-day-sun .calendar__day:first-child { | ||
| margin-left: calc((100px + 1px) * 6); | ||
| } | ||
|
|
||
| .calendar--start-day-wed .calendar__day:first-child { |
There was a problem hiding this comment.
You must generate all calendar--start-day-* modifiers (mon, tue, wed, thu, fri, sat, sun) using @each as required in the description; currently only sun and wed are implemented manually, so changing other start-day modifiers will not affect the layout, violating checklist item #1.
| .calendar--month-length-28 .calendar__day:nth-child(n + 29) { | ||
| display: none; | ||
| } | ||
|
|
||
| .calendar--month-length-29 .calendar__day:nth-child(n + 30) { | ||
| display: none; | ||
| } | ||
|
|
||
| .calendar--month-length-30 .calendar__day:nth-child(n + 31) { |
There was a problem hiding this comment.
The month-length modifiers (28, 29, 30, 31) should be created using @for, and there should also be a rule for calendar--month-length-31; right now 28–30 are hardcoded and 31 is missing, which does not match the description and breaks checklist item #1 when changing month-length.
| .calendar { | ||
| display: flex; | ||
| flex-wrap: wrap; | ||
| gap: 1px; | ||
| width: calc(100px * 7 + 1px * 6); | ||
| } | ||
|
|
||
| .calendar__day { | ||
| box-sizing: border-box; | ||
| width: 100px; | ||
| height: 100px; | ||
| border: 1px solid #000; | ||
| background-color: #eee; |
There was a problem hiding this comment.
Repeated hardcoded values like 100px and 1px must be replaced with SCSS variables as per "Don't use hardcoded px values if they are used several times"; consider defining variables for day size, border width, and gap and reusing them in width, height, margin, and transform calculations.
| .calendar__day { | ||
| box-sizing: border-box; | ||
| width: 100px; | ||
| height: 100px; | ||
| border: 1px solid #000; | ||
| background-color: #eee; | ||
|
|
||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| transition-duration: 0.5s; | ||
|
|
||
| font-family: Arial, sans-serif; | ||
| font-size: 30px; |
There was a problem hiding this comment.
The centering of day numbers is implemented by making .calendar__day a flex container, but the requirement is to add the number in the center using ::before; adjust styles so the pseudo-element is responsible for the displayed number and its centering, not the element content itself.
mateacademy-ai-mentor
left a comment
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
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 logic issue with the start-day behavior that breaks a core requirement. Your calendar currently uses calendar--start-day-sun as the default in src/index.html, but in src/styles/main.scss the $start-days map assigns sun: 6, which shifts a Sunday-start month so that day 1 appears in the 7th column instead of the first; this violates the requirement that Sunday is the first column and that changing the start-day modifier moves day 1 to the correct column. This misalignment means the start-day modifiers don’t behave as specified in the task description and checklist [CHECKLIST ITEM #1], so functionality is not fully correct yet.
To fix this, update your $start-days map in src/styles/main.scss so that the offsets are 0-based from Sunday: sun: 0, mon: 1, tue: 2, …, sat: 6. That way, the generated margin-left for .calendar--start-day-sun will be 0, and other modifiers (like calendar--start-day-mon, calendar--start-day-wed, etc.) will place day 1 in the appropriate column consistent with “Monday is the 1st, Friday is the 5th” as described. Once this mapping is corrected, your implementation should align with all core functional and checklist requirements, since the rest of the layout, hover behavior, SCSS variables, and BEM usage are already in good shape. Keep going—this is a small but important tweak to an otherwise solid solution.
✨ 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 uses calendar--start-day-sun by default, but your $start-days map assigns sun: 6, so the generated rule applies a large margin-left to the first day; this violates the requirement that a Sunday-start month should place day 1 in the first column (no extra shift) and that “Monday is the 1st, Friday is the 5th” (your offsets are all shifted by one). Consider remapping offsets so sun has 0 (first column), mon has 1, tue 2, etc., to match the task’s column expectations.
| $start-days: ( | ||
| mon: 0, | ||
| tue: 1, | ||
| wed: 2, | ||
| thu: 3, | ||
| fri: 4, | ||
| sat: 5, | ||
| sun: 6, |
There was a problem hiding this comment.
This start-days map assigns sun: 6, which shifts a Sunday-start month so the first day appears in the 7th column. This violates the requirement that “Set calendar to start from Sunday by default” with Sunday as the first column; sun should have offset 0 (and other days adjusted) so that changing the calendar--start-day-* modifiers moves the first day to the correct column as per the description and checklist item “Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout”.
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 so you can move forward while continuing to learn and improve.
You did a strong job on most of the requirements: the calendar has 31 .calendar__day elements without numbers in the HTML, the numbers are generated via ::before and @for, the layout uses flex and calculated widths, hover behavior matches the spec, and your BEM structure and SCSS organization are clean and readable. The main remaining issue is the start-day logic in src/styles/main.scss: your $start-days map is currently Monday-based (mon: 0 ... sun: 6), which causes the calendar--start-day-sun modifier to place day 1 in the 7th column instead of the 1st, so changing the start-day modifier does not correctly realign the first day as required [CHECKLIST ITEM #1]. To fix this, update the map so that Sunday is index 0 and the rest follow in order, for example: sun: 0, mon: 1, tue: 2, wed: 3, thu: 4, fri: 5, sat: 6, so that the margin-left calculation for the first .calendar__day correctly reflects the starting weekday.
Overall, your work shows good understanding of SCSS, layout, and BEM; once you adjust the $start-days mapping, your calendar will fully satisfy the dynamic start-day behavior, and this is a great step forward in mastering layout logic.
✨ 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--start-day-sun calendar--month-length-31"> |
There was a problem hiding this comment.
This calendar element correctly uses calendar--start-day-sun and calendar--month-length-31 as default modifiers, but due to the current $start-days map, start-day-sun does not actually place day 1 in the first (Sunday) column, which violates checklist item #1: Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout and the requirement that Sunday is the first column.
| $start-days: ( | ||
| 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" because the $start-days offsets are not 0-based from Sunday. With sun: 6, the default calendar--start-day-sun will shift the first day into the 7th column instead of the first, so Sunday is not the first column and other days (like Monday and Friday) won’t align to the 2nd and 5th columns as required. Update the map to sun: 0, mon: 1, tue: 2, wed: 3, thu: 4, fri: 5, sat: 6 so the margin-left correctly reflects the starting weekday.
2 participants
