Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on the overall structure and use of SCSS features! I am requesting a few changes to align the project with the task's core requirements. The main issue is that the day elements in index.html use modifier classes like calendar__day--1, which violates a key checklist item [CHECKLIST ITEM #2]. Please remove these modifiers, leaving only class="calendar__day". To fix the styling, you'll need to refactor src/styles/blocks/calendar.scss to use CSS counters for numbering the days and the :first-child pseudo-class to apply the starting margin. Also, please remove the !important flag from src/styles/index.scss, as it's unnecessary and goes against the style guide [CHECKLIST ITEM #4]. After these adjustments, your solution will be perfect!
✨ 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/blocks/calendar.scss
Outdated
| @for $day from 1 through 31 { | ||
| &--#{$day}::before { | ||
| content: '#{$day}'; | ||
| font-family: Arial, sans-serif; | ||
| font-size: 30px; | ||
| } | ||
| } |
There was a problem hiding this comment.
This loop generates modifier classes for each day (e.g., calendar__day--1, calendar__day--2, etc.) to add the numbers. This approach violates a key requirement from the checklist: "Each day has no modifiers, only class (eg. calendar__day)".
Consider using CSS counters to add the numbers. You would initialize a counter on the parent .calendar element and then increment and display it on the ::before pseudo-element of each .calendar__day.
src/styles/blocks/calendar.scss
Outdated
|
|
||
| @each $name, $col in $start-days { | ||
| .calendar--start-day-#{$name} { | ||
| .calendar__day--1 { |
There was a problem hiding this comment.
This selector targets .calendar__day--1, which relies on a modifier class that shouldn't be present on the day elements, as per the task requirements.
To apply styles to the first day of the calendar, you should use the :first-child pseudo-class instead. This allows you to target the element without needing an extra class.
src/styles/index.scss
Outdated
| display: flex !important; | ||
| justify-content: center !important; | ||
| align-items: center !important; |
There was a problem hiding this comment.
The !important flag should be avoided as it can lead to issues with style specificity and makes debugging harder. These styles will work correctly without !important.
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; |
There was a problem hiding this comment.
These styles for centering are redundant because the body element is already configured to center its children. You can remove these flex properties from the .page class to simplify the code.
src/index.html
Outdated
| <div class="calendar__day calendar__day--1"></div> | ||
| <div class="calendar__day calendar__day--2"></div> | ||
| <div class="calendar__day calendar__day--3"></div> | ||
| <div class="calendar__day calendar__day--4"></div> | ||
| <div class="calendar__day calendar__day--5"></div> | ||
| <div class="calendar__day calendar__day--6"></div> | ||
| <div class="calendar__day calendar__day--7"></div> | ||
| <div class="calendar__day calendar__day--8"></div> | ||
| <div class="calendar__day calendar__day--9"></div> | ||
| <div class="calendar__day calendar__day--10"></div> | ||
| <div class="calendar__day calendar__day--11"></div> | ||
| <div class="calendar__day calendar__day--12"></div> | ||
| <div class="calendar__day calendar__day--13"></div> | ||
| <div class="calendar__day calendar__day--14"></div> | ||
| <div class="calendar__day calendar__day--15"></div> | ||
| <div class="calendar__day calendar__day--16"></div> | ||
| <div class="calendar__day calendar__day--17"></div> | ||
| <div class="calendar__day calendar__day--18"></div> | ||
| <div class="calendar__day calendar__day--19"></div> | ||
| <div class="calendar__day calendar__day--20"></div> | ||
| <div class="calendar__day calendar__day--21"></div> | ||
| <div class="calendar__day calendar__day--22"></div> | ||
| <div class="calendar__day calendar__day--23"></div> | ||
| <div class="calendar__day calendar__day--24"></div> | ||
| <div class="calendar__day calendar__day--25"></div> | ||
| <div class="calendar__day calendar__day--26"></div> | ||
| <div class="calendar__day calendar__day--27"></div> | ||
| <div class="calendar__day calendar__day--28"></div> | ||
| <div class="calendar__day calendar__day--29"></div> | ||
| <div class="calendar__day calendar__day--30"></div> | ||
| <div class="calendar__day calendar__day--31"></div> |
There was a problem hiding this comment.
This violates checklist item #2: 'Each day has no modifiers, only class (eg. calendar__day)'.
You have added modifiers like calendar__day--1, calendar__day--2, etc. to each day element. These should be removed, leaving only the calendar__day class on each div. You can use the :nth-child() pseudo-selector in your SCSS to target specific days for numbering and applying the initial margin.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Excellent work on this task! You've successfully addressed all the major feedback from the previous review, and the calendar functionality is implemented perfectly. I am approving your solution as it meets all the core requirements.
For future reference, please pay attention to a couple of minor code style points in src/styles/index.scss:
- The styles for
bodyand.pageare duplicated. To follow the DRY (Don't Repeat Yourself) principle, you can apply these centering styles to just one selector. - There is some commented-out code that should be removed to keep the codebase clean.
These are minor points that don't block the solution, but cleaning them up is good practice. Great job overall!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| .page { | ||
| min-height: 100vh; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; |
There was a problem hiding this comment.
The styles applied to the .page class are redundant because the body element already contains the same Flexbox properties for centering. Since .page is the direct child of body, these styles on .page are not necessary. Removing this rule will simplify the code and adhere to the DRY (Don't Repeat Yourself) principle.
| // display: flex; | ||
| // align-items: center; | ||
| // justify-content: center; | ||
|
|
||
| // min-height: 100vh; | ||
| // display: grid; | ||
| // place-items: center; |
There was a problem hiding this comment.
Please remove this commented-out code. It's a good practice to keep the final version of your code clean from unused fragments.
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 ❗️