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 this calendar! Your use of BEM and SCSS features is well-executed. However, a few adjustments are needed to meet all the project's requirements.
Here are the key issues to address:
- Animation Duration: The hover animation for the
transformproperty is set to2s, but the requirement is0.5sfor all transitions. - Sass Calculation Error: The
margin-leftcalculation for the starting day adds a unitless number to a pixel value (($index * $field-size) + $index), which will cause the Sass compilation to fail. Please ensure the units are compatible, for example, by adding the gap width explicitly:($index * 1px). - Layout and Sizing: The calendar has a fixed height, which prevents it from adapting to months with fewer weeks. The height should be determined by the content. Additionally, the total width calculation is missing the required
10pxof padding on the container. - File Naming: The task requires the stylesheet to be named
main.scss, but it's currentlyindex.scss. Please rename the file and update the<link>in your HTML.
Once these points are addressed, your solution will be in great shape. Keep up the great work!
✨ 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-size: 706px; | ||
| $field-size: calc(($calendar-size - 6px) / 7); |
There was a problem hiding this comment.
The calculation here is a bit indirect. The primary requirement is that each day is a 100px square. It would be clearer to define the size of a day first (e.g., $field-size: 100px;), and then calculate the total calendar width based on that. This makes the code's intent more obvious and easier to maintain.
src/styles/index.scss
Outdated
|
|
||
| .calendar { | ||
| width: $calendar-size; | ||
| height: $calendar-size; |
There was a problem hiding this comment.
You've set a fixed height for the calendar, making it a square. This doesn't adapt to the content, which can have a varying number of rows (e.g., 5 or 6) depending on the month's start day. A fixed height can create unnecessary empty space inside the calendar block. The height should be determined by its content.
src/styles/main.scss
Outdated
| background-color: $grey; | ||
| transition: | ||
| background-color 0.5s, | ||
| transform 2s; |
There was a problem hiding this comment.
The task requires all hover animations to have a duration of 0.5s. The transform animation is currently set to 2s.
src/styles/index.scss
Outdated
| @each $day, $index in $days { | ||
| &--start-day-#{$day} { | ||
| .calendar__day:first-child { | ||
| margin-left: ($index * $field-size) + $index; |
There was a problem hiding this comment.
This calculation will likely fail during Sass compilation. You are trying to add a unitless number ($index) to a pixel value ($index * $field-size), which is not allowed. To fix this, you need to ensure the units are compatible. You should add the total width of the gaps, which can be calculated as $index * 1px.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work addressing the previous feedback! Your solution is nearly perfect, but I'm requesting one change to meet a core requirement. The task specifies that all hover animations must have a duration of 0.5s, but the transform property's transition is still set to 2s in src/styles/main.scss. Please update this value to match the requirement. As a minor suggestion, consider creating a variable for the 1px value used for gaps and borders to improve maintainability. Keep up the great work!
✨ 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
| $grey: #eee; | ||
| $pink: #ffbfcb; | ||
| $calendar-size: 706px; | ||
| $field-size: calc(($calendar-size - 6px) / 7); |
There was a problem hiding this comment.
According to the requirement 'Don't use hardcoded px values if they are used several times', it would be better to create a variable for the 1px gap/border size, for example: $gap: 1px;.
You could then use this variable to calculate the 6px value here (e.g., 6 * $gap), for the gap property on line 28, the border on line 33, and in the margin-left calculation on line 69. This improves maintainability.
src/styles/main.scss
Outdated
| background-color: $grey; | ||
| transition: | ||
| background-color 0.5s, | ||
| transform 2s; |
There was a problem hiding this comment.
The requirement states that all hover changes should be animated with a duration of 0.5s. The transform property still has a duration of 2s. Please ensure all transitions have the same 0.5s duration to meet the requirement.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this revision! You've successfully addressed the feedback from the previous review by correcting the animation duration. Your solution now meets all the requirements perfectly. The SCSS is clean, well-structured, and makes great use of variables, loops, and the BEM methodology. This is a fantastic submission, and I am approving 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
DEMO LINK
TEST REPORT LINK
Changing 'month-lengh' and 'start-day' modifier in the code element
reflects in changing calendar layout
Each day has no modifiers, only class (eg. calendar__day)
All
Typical MistakesfromBEMlesson theory are checked.Code follows all the Code Style Rules ❗️