Skip to content

add task solution#5208

Open
spanchenko09 wants to merge 4 commits intomate-academy:masterfrom
spanchenko09:develop
Open

add task solution#5208
spanchenko09 wants to merge 4 commits intomate-academy:masterfrom
spanchenko09:develop

Conversation

@spanchenko09
Copy link

Copy link

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

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 core requirement about the calendar layout is not working correctly and this directly violates [CHECKLIST ITEM #1]. Your HTML and most of the SCSS are implemented well: the calendar is centered, days are styled and numbered correctly via ::before, hover animations work, and month-length modifiers correctly hide extra days.

The critical blocker is in src/styles/main.scss: the $start-days map currently uses mon: 0, tue: 1, ..., sun: 6, which makes the calendar--start-day-sun modifier push the first day into the last column instead of the Sunday column. The requirement says that changing the start-day modifier must move the first day into the correct weekday column, so the offset values must represent “how many empty slots are before the 1st.” For a Sunday-first layout, this means something like:

$start-days: (
  sun: 0,
  mon: 1,
  tue: 2,
  wed: 3,
  thu: 4,
  fri: 5,
  sat: 6,
);

or you should adjust the calculation so that the applied margin/offset results in the first visible day landing under the intended weekday label. After you fix this mapping (and ensure your margin/position formula uses that offset correctly), the start-day modifiers should visually align the first day with the expected column. As a minor, non-blocking improvement, you could also reuse your $anim-time variable in the transition and avoid duplicating the 0.5s value, but that is not required for acceptance. Overall, you are very close—once the start-day offset logic is corrected, the solution will meet the functional 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 on lines 74 to 86
$start-days: (
mon: 0,
tue: 1,
wed: 2,
thu: 3,
fri: 4,
sat: 5,
sun: 6,
);

@each $name, $idx in $start-days {
&--start-day-#{$name} &__day:first-child {
margin-left: ($day-size + $gap) * $idx;

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" — the $start-days map currently assigns mon: 0, tue: 1, ..., sun: 6, which causes calendar--start-day-sun to shift the first day to the last column instead of placing it in the Sunday column. Change the map so the index equals the number of empty slots before the 1st, for example: (sun: 0, mon: 1, tue: 2, wed: 3, thu: 4, fri: 5, sat: 6), or adjust the calculation accordingly so the first day lands in the expected weekday column.

Copy link

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

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, although your HTML and SCSS correctly implement all of the specified calendar features, the visual regression tests are currently failing in four scenarios, which is a blocking issue for this task. The calendar structure (31 .calendar__day elements, modifiers for start day and month length, hover behavior, and generated numbers via ::before) all match the functional requirements well, so your core implementation is solid. However, BackstopJS reports pixel mismatches, which means the rendered layout (spacing, offsets for different start days, hidden days for month lengths, or hover visuals) does not exactly match the reference images the pipeline checks against.

To move forward, you should run the Backstop tests locally, open backstop_data/html_report/index.html, and inspect each failed_diff_*.png to see what is different (e.g., day box positions, gaps, font alignment, or hover transform). Then, adjust your SCSS values (such as gap, padding, border size, transform distance, or nth-child visibility rules for different month lengths) until the rendered output matches the references closely enough that the tests pass. Once those visual differences are resolved and Backstop reports all scenarios as passing, your solution will fully meet both the functional requirements and the CI standards. Your current work is very close; you mainly need to fine-tune the visuals to align with the automated reference images.


✨ 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

Copy link

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

Choose a reason for hiding this comment

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

Review Summary

I am approving your solution as an educational override so you can keep progressing while you practice and refine these details. You’ve done a strong job implementing the SCSS requirements: the flex grid, generated day numbers, hover animations, and BEM structure are all in place and follow the spec well. There are, however, two important points to correct next:

  1. The $start-days mapping in src/styles/main.scss is inverted [CHECKLIST ITEM #1]. Right now it maps 'mon': 0'sun': 6, which makes a calendar--start-day-sun modifier apply a large left margin instead of none; reverse it so 'sun': 0, 'mon': 1, … 'sat': 6 and the first day will land in the correct column.
  2. The cursor behavior does not fully match the requirement that it becomes a pointer only on hover. At the moment cursor: pointer; is set on the base .calendar__day rule (around line 48); remove it there and move cursor: pointer; into the &:hover block so the cursor changes only when the user actually hovers a cell.

Once you adjust these, your calendar should both look correct and align perfectly with the interaction requirements—overall, you’re very close and your SCSS structure shows solid understanding, so keep up the good work as you refine these last details.


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

Footnotes

  1. Rate AI review example

Comment on lines +73 to +81
$start-days: (
'mon': 0,
'tue': 1,
'wed': 2,
'thu': 3,
'fri': 4,
'sat': 5,
'sun': 6,
);

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'. The $start-days mapping is inverted (you have 'mon': 0 ... 'sun': 6). That makes calendar--start-day-sun apply a large margin instead of none. Reverse the mapping so that 'sun' => 0, 'mon' => 1, 'tue' => 2, 'wed' => 3, 'thu' => 4, 'fri' => 5, 'sat' => 6 (so margin-left = ($day-size + $gap) * index yields correct column).

Comment on lines +48 to +64
cursor: pointer;
transition:
transform $anim-time,
background $anim-time;

&::before {
pointer-events: none;
line-height: 1;
}

&:hover {
background: $hover-bg;
transform: translateY(-20px);
transition:
background $anim-time,
transform $anim-time;
}

Choose a reason for hiding this comment

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

The spec requires: 'On hovering over a cell: cursor should become pointer'. You set cursor: pointer; on the base .calendar__day (line 48) so the pointer appears even when not hovering. Remove cursor from the base rule and add cursor: pointer; inside the &:hover block so the cursor changes only on hover.

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