Skip to content

add task solution#5459

Open
SylwiaLeung wants to merge 2 commits into
mate-academy:masterfrom
SylwiaLeung:develop
Open

add task solution#5459
SylwiaLeung wants to merge 2 commits into
mate-academy:masterfrom
SylwiaLeung:develop

Conversation

@SylwiaLeung

Copy link
Copy Markdown
  • 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 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 a few explicit task requirements are still not met, even though the core calendar functionality works well. First, the styles are currently in src/styles/index.scss and linked from index.html, but the task requires using src/styles/main.scss; please move/rename your SCSS to main.scss and update the <link> href accordingly. Second, the calendar width must be exactly 7 * day-size + 6 * gap + 2 * 10px padding, and this padding should use a SCSS variable (for example $calendar-padding) instead of being hardcoded in multiple places—update your $calendar-week-size calculation to include this padding and reuse the variable where you set padding. Third, the hover animation requirement states that all hover changes should be animated for 0.5s; while cursor cannot be animated, you should move the transition declaration to the base .calendar__day (not inside :hover) so transform and background-color animate smoothly both on hover in and hover out. Functionally, your layout, day numbering, start-day and month-length modifiers, and overall BEM usage look solid, so once these path, width, and transition issues are addressed, the solution will fully match the task. Keep up the good work—these are targeted fixes that will bring your implementation perfectly in line with the requirements.


✨ 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/main.scss
align-items: center;
justify-content: center;
}

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: "Write styles in src/styles/main.scss instead of src/style.css". Your HTML links to styles/index.scss, and your styles are in index.scss. To match the requirement, you need to place styles in main.scss and update this href to point to that file.

Comment thread src/styles/index.scss Outdated
Comment on lines +3 to +15
$calendar-week-size: calc($calendar-day-size * 7 + $calendar-gap-size * 6);

body {
margin: 0;
min-height: 100vh;
display: flex;
align-items: center;
justify-content: center;
}

.calendar {
gap: $calendar-gap-size;
width: $calendar-week-size;

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 description requires "limit its width to exactly 7 columns + 10px paddings" and "Don't use hardcoded px values if they are used several times". Here width is only 7 * day-size + 6 * gap and does not include left/right paddings. Also, the paddings are hardcoded later as 10px without a variable. Introduce a padding variable and include it in the width calculation so the total width matches the requirement.

Comment thread src/styles/index.scss Outdated

display: flex;
flex-wrap: wrap;
padding: 10px;

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 set padding: 10px; but the width above does not account for this padding. The task states the calendar width must be "7 columns + 10px paddings". Use a variable for padding and include it in the $calendar-week-size calculation rather than a separate hardcoded 10px here.

Comment thread src/styles/index.scss Outdated
Comment on lines +63 to +69
&:hover {
cursor: pointer;
transform: translateY(-20px);
background-color: #ffbfcb;
transition:
transform 0.5s,
background-color 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.

The hover requirement says "All changes should be animated with the duration of 0.5s". Currently the transition only covers transform and background-color, and it is declared inside :hover. Consider moving transition to the base .calendar__day so it consistently animates these properties when entering and leaving hover. (Note: cursor cannot be animated, but the other changes should all be covered by a single transition.)

@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 fully meets the functional and technical requirements of the calendar task. The calendar is correctly centered, uses a flex layout with the proper gap and width calculation for seven columns, and contains exactly 31 .calendar__day elements with no extra modifiers on individual days. Day numbers are generated purely via SCSS using ::before and @for, and the start-day and month-length BEM modifiers correctly shift the first day and hide trailing days, so changing these modifiers in HTML properly updates the layout [CHECKLIST ITEM #1]. The hover behavior (pointer cursor, pink background, and smooth upward movement over 0.5s) is implemented on the base day element, matching the animation requirement. You also followed the structural constraints: styles are in src/styles/main.scss, linked correctly from index.html, and there is no JavaScript. BEM naming is consistent, repeated pixel values are factored into variables, and there are no checklist violations. Nice work bringing everything in line with the specification and previous feedback; this is a clean, standards-compliant implementation.


✨ 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