done - #5320
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on the overall structure and use of SCSS features! The BEM methodology is well-applied and the calendar looks good.
However, this requires changes because of a critical bug that breaks a core requirement. The starting day for Saturday is incorrect because its value in the $start-day map is 5 when it should be 6. This causes Saturday to start in the same column as Friday, which breaks the layout functionality mentioned in the requirements [CHECKLIST ITEM #1].
Additionally, the task requires the stylesheet to be named src/styles/main.scss, but it is currently src/styles/index.scss. Please also address the unused variables with typos ($calendar-heigh and $month-lenth) to improve code quality. After fixing these issues, your project will be in great shape!
✨ 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__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 index for Saturday (sat) should be 6, not 5. With the current value, both Friday and Saturday will start in the 5th column, which is incorrect.
| <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.
This variable has a typo ('lenth' instead of 'length') and is not used in the code. The @for loop that generates the month-length modifiers uses hardcoded numbers. It's better to remove unused variables to avoid confusion.
| $days-in-week: 7; | ||
| $border: 1px solid black; | ||
| $calendar-width: $cell-size * $days-in-week + $gap * ($days-in-week - 1) + $padding * 2; | ||
| $calendar-heigh: $cell-size * 5 + $gap * 4 + $padding * 2; |
There was a problem hiding this comment.
This variable is declared but never used in the stylesheet. It's a good practice to remove unused variables to keep the code clean and easier to maintain.
| wed: 3, | ||
| thu: 4, | ||
| fri: 5, | ||
| sat: 5, |
There was a problem hiding this comment.
The index for Saturday appears to be incorrect. It should be the 6th day of the week, but it's currently set to 5, the same as Friday. This will cause the calendar to start on the wrong day when start-day-sat is used.
| $month-lenth: ( | ||
| 28, | ||
| 29, | ||
| 30, | ||
| 31 | ||
| ); |
There was a problem hiding this comment.
This variable is declared but not used. The @for loop on line 80 uses hardcoded values instead. To improve the code, you could either remove this unused variable or refactor the loop to use it. Also, note the typo in the variable name; it should be month-length.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on fixing the bug with the calendar's start day—the functionality is now perfect! However, one critical requirement from the task description still needs to be addressed before this can be approved. The stylesheet must be named src/styles/main.scss, but it is currently src/styles/index.scss.
Please rename the file and update the corresponding <link> tag in your index.html to point to the correct path. After that small change, your solution will be complete!
✨ 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
| @@ -1,3 +1,81 @@ | |||
| body { | |||
There was a problem hiding this comment.
According to the task description, this file should be named main.scss. Please rename the file and update the corresponding <link> in your HTML file.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work! You've successfully addressed all the feedback from the previous review. Your SCSS is clean, well-structured, and makes great use of variables and loops for the calendar logic. The BEM naming is implemented perfectly, and the final result functions exactly as required. This is a fantastic submission, and I'm happy to approve it. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.