Skip to content

Develop#5477

Open
IvanVarvaruk wants to merge 4 commits into
mate-academy:masterfrom
IvanVarvaruk:develop
Open

Develop#5477
IvanVarvaruk wants to merge 4 commits into
mate-academy:masterfrom
IvanVarvaruk:develop

Conversation

@IvanVarvaruk

Copy link
Copy Markdown
  • DEMO LINK

  • TEST REPORT LINK

  • Changing 'month-lengh' and 'start-day' modifier in the code element

  • Each day has no modifiers, only class (eg. calendar__day)

  • All Typical Mistakes from BEM lesson theory are checked.

  • Code follows all the Code Style Rules ❗️

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there are a few critical points where the current implementation does not fully meet the task requirements. First, the hover animation is not guaranteed to apply to both background-color and transform, since you only set transition-duration: 0.5s without specifying properties; you should define an explicit transition such as transition: background-color 0.5s, transform 0.5s; on .calendar__day to satisfy the “all hover changes are animated for 0.5s” requirement. Second, your $week-days map in src/styles/utils/varibles.scss currently assigns 'mon': 0 and 'sun': 6, which means calendar--start-day-mon causes no offset and calendar--start-day-sun adds a 6-column offset; for a Sunday-start calendar this must be reversed so that Sunday is 0, Monday is 1, and so on, otherwise [CHECKLIST ITEM #1] (“month should start at the correct column”) is violated. Third, the main stylesheet is src/styles/index.scss and linked as such in HTML, while the task explicitly requires using src/styles/main.scss as the entry SCSS file; you need to move/rename the file and update the <link> to match the required structure.

Once you fix the transition definition, correct the weekday offsets in $week-days so that changing calendar--start-day-... positions the first day in the right column, and use the required main.scss entry file, your solution should fully satisfy the functional and structural requirements. As minor improvements, you could explicitly set font-family: Arial, sans-serif for the day numbers and consider introducing variables for repeated text metrics like the 30px font size, but these are not blockers. You’ve done a solid job with BEM, SCSS, modifiers, and overall layout; addressing these specific points will bring the implementation fully in line with the checklist and description.


✨ 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. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/styles/blocks/calendar.scss Outdated
box-sizing: border-box;
background-color: #eee;
border: 1px solid #000;
transition-duration: 0.5s;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transition-duration: 0.5s; alone does not guarantee that the background-color and transform changes will be animated; to meet the requirement "All changes on hover should be animated with the duration of 0.5s", define a full transition (e.g. transition: background-color 0.5s, transform 0.5s;).

Comment thread src/styles/main.scss
}

@import 'utils/varibles';
@import 'blocks/calendar';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You only set transition-duration: 0.5s; without specifying properties; to ensure all hover changes (background-color and transform) are animated as required, define a full transition with the relevant properties.

Comment thread src/styles/utils/varibles.scss Outdated
$cell-size: 100px;
$gap: 1px;
$week-days: (
'mon': 0,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item #1: "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout" and the description "The month should start at the correct column (Monday is the 1st, Friday is the 5th)" because your $week-days map assigns 'mon': 0 and 'sun': 6, which makes calendar--start-day-mon produce no offset and calendar--start-day-sun a 6-column offset; for a calendar starting from Sunday by default, Sunday should correspond to zero offset and Monday to one column.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because one explicit task requirement is still not met: all visual changes on hover must be animated with a duration of 0.5s, but your .calendar__day currently uses 0.25s for background-color and transform. The rest of the calendar behavior and structure looks solid: you correctly use SCSS and BEM, generate the day numbers with ::before and @for, implement start-day and month-length modifiers via maps/loops, and the layout responds correctly when those modifiers change. To fix the remaining blocker, go to src/styles/blocks/calendar.scss and update the transition rule for .calendar__day to:

  • transition: background-color 0.5s, transform 0.5s;

This aligns the hover animation timing with the specification “All changes should be animated with the duration of 0.5s” and with the previous review’s explicit request. Once you adjust that duration, your solution will fully comply with the task requirements, and the implementation should be ready to pass. Keep the rest of your structure as-is; you’ve done a good job on the SCSS logic and BEM usage.


✨ 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. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/styles/blocks/calendar.scss Outdated
Comment on lines +14 to +16
transition:
background-color 0.25s,
transform 0.25s;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires “All changes should be animated with the duration of 0.5s”, and previous review explicitly asked for transition: background-color 0.5s, transform 0.5s;. Using 0.25s here violates that requirement; update both transitions to 0.5s to comply.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Your solution is approved because it meets all the key functional and technical requirements for the calendar: the layout is centered, the days are generated via SCSS with ::before and @for, the start-day and month-length modifiers work correctly, and the hover behavior (pointer, #ffbfcb, translateY(-20px), 0.5s transition) is implemented exactly as required. You respected BEM conventions, kept all day cells as plain calendar__day elements without extra modifiers, and correctly wired SCSS through main.scss with variables for repeated values like $cell-size and $gap, which is great practice.

The only minor improvement to consider is making line-height depend on $cell-size (for example line-height: $cell-size - 2px) instead of a hardcoded 98px, so your typography scales automatically with the cell size. This is a small refinement and not blocking, but it will make your layout more flexible and consistent. Overall, you’ve done a solid job translating the checklist requirements into clean SCSS and HTML, and this is strong progress in both BEM structure and SCSS techniques.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants