Skip to content

Layout-calendar#5465

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

Layout-calendar#5465
olegpuzin wants to merge 2 commits into
mate-academy:masterfrom
olegpuzin:develop

Conversation

@olegpuzin

Copy link
Copy Markdown

@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 two explicit task requirements are still not fully met, even though the rest of your solution is implemented very well. First, the specification says styles must be written in src/styles/main.scss, but your entry stylesheet is src/styles/index.scss and index.html links to styles/index.scss; you need to rename index.scss to main.scss, keep the imports, and update the <link> href accordingly so the entry file name matches the requirement. Second, the calendar width must be "exactly 7 columns + 10px paddings" and you should avoid repeating hardcoded px values; right now the width is calculated as 7 * $widthDay + 6 * $gapDay and the 10px padding is separate and hardcoded, so paddings are not part of the width formula and the 10px literal is used multiple times. Introduce a padding variable (for example $calendar-padding: 10px; in variables.scss), use it in padding, and include it in the width calculation (e.g. width: 7 * $widthDay + 6 * $gapDay + 2 * $calendar-padding;) so the total horizontal size is clearly "7 columns + left+right padding" expressed via variables. These are critical because they directly violate the task description and checklist about the entry SCSS file and the width/padding calculation, even though all other behaviors (31 days, modifiers, hover effects, BEM, @for/@each, flex centering) look correct. Once you fix the stylesheet entry name and the padding-aware width formula using variables, your solution should fully satisfy the requirements and can be accepted.


✨ 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/calendar.scss
position: relative;
width: $widthDay;
height: $heightDay;
border: 1px solid black;

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: "Write styles in src/styles/main.scss instead of src/style.css". Currently you use styles/index.scss as the main stylesheet and link to it here. This violates that requirement; consider renaming your entry SCSS file to main.scss and updating this href accordingly.

Comment thread src/styles/main.scss
Comment on lines +2 to +3
@import './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.

The requirement says: "Use flex with 1px gap and limit its width to exactly 7 columns + 10px paddings" and also "Don't use hardcoded px values if they are used several times" and "Use properly named variables to make all the calculations more clear." Here width is 7 * day + 6 * gap and padding 10px is separate and hardcoded. Consider introducing a padding variable and calculating width as 7 columns + 6 gaps + both paddings using variables, so the total width explicitly includes padding and avoids repeated literal 10px values.

Comment thread src/styles/variables.scss
Comment on lines +2 to +3
$heightDay: $widthDay;
$gapDay: 1px;

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 the requirement: "Use flex with 1px gap and limit its width to exactly 7 columns + 10px paddings" and "Don't use hardcoded px values if they are used several times". The width is calculated as width: $widthDay * 7 + $gapDay * 6; but does not include the 10px paddings, and padding: 10px; uses a repeated hardcoded value. Consider introducing a padding variable (e.g. $calendarPadding) and including it in the width calculation so the total width matches 7 day columns + left+right padding, while avoiding repeated literal 10px values.

@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 correctly implements all the required calendar functionality and matches the technical constraints from the task. The calendar is centered with flexbox, uses exactly 31 .calendar__day elements without extra modifiers, and day numbers are generated via SCSS with ::before and an @for loop as required. The calendar--start-day-* and calendar--month-length-* modifiers work correctly, shifting the first day and hiding extra days purely through SCSS, and the hover effect (pointer, pink #FFBFCB, 20px upward transform, 0.5s transition) behaves as specified. SCSS is structured properly with main.scss as the entry point, partials for variables and calendar styles, and consistent BEM naming. The only issue mentioned is a tiny visual regression difference in the hover test (0.53% vs 0.5%), which is non-critical and does not indicate broken functionality; you can handle reference image updates or minor pixel tweaks separately if needed. Overall, your implementation satisfies the task requirements and is ready to move forward—nice work bringing everything in line with the description and checklist.


✨ 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

@vadiimvooo vadiimvooo 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.

Well done

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.

3 participants