Skip to content

Comments

Develop#5196

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

Develop#5196
Roma-Yamshchikov wants to merge 4 commits intomate-academy:masterfrom
Roma-Yamshchikov:develop

Conversation

@Roma-Yamshchikov
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 there are a few critical requirements that are not yet met and will break automated checks and core functionality. First, the stylesheet filename and link are incorrect: the task explicitly requires styles in src/styles/main.scss, but your file is src/styles/index.scss and src/index.html links to styles/index.scss; rename the file to main.scss and update the <link> in src/index.html accordingly so the checker can find it. Second, the $weekdays map in src/styles/index.scss currently uses mon: 0 ... sun: 6, which makes the calendar--start-day-* modifiers place the first day in the wrong column; update the mapping to sun: 0, mon: 1, tue: 2, wed: 3, thu: 4, fri: 5, sat: 6 so the month starts in the correct column for each modifier. Third, the transition is defined only on .calendar__day:hover, so the hover-out is not smoothly animated; move the transition: background-color 0.5s, transform 0.5s; (or similar) to the base .calendar__day rule so both entering and leaving hover animate for 0.5s as required. As a smaller improvement (not blocking), consider introducing a $container-padding variable and including it in the width calculation so the calendar width is exactly 7 columns plus 10px total padding, and avoid repeating hardcoded padding values. Once you address the filename/link, weekday mapping, and transition placement, your solution should satisfy the functional and checklist requirements, and the rest of your structure and SCSS usage looks solid.


✨ 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

src/index.html Outdated
@@ -12,7 +13,40 @@
href="styles/index.scss"

Choose a reason for hiding this comment

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

This violates the requirement "Write styles in src/styles/main.scss instead of src/style.css." The HTML currently links to styles/index.scss (line shown). Rename your stylesheet to main.scss under src/styles/ and update the link to styles/main.scss so it matches the task requirement.

@@ -1,3 +1,65 @@
$cell-size: 100px;

Choose a reason for hiding this comment

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

This violates the requirement "Write styles in src/styles/main.scss instead of src/style.css." Your stylesheet is src/styles/index.scss. Rename this file to main.scss and update the HTML link to styles/main.scss so the task's required filename is used and automated checks find it.

Comment on lines 4 to 12
$weekdays: (
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.

The $weekdays mapping is reversed which causes start-day modifiers to place the first day in the wrong column. The spec requires that "The month should start at the correct column (Monday is the 1st, Friday is the 5th)." Also check the checklist item: "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout" — with the current mapping mon: 0 ... sun: 6 the positions are incorrect. Use the mapping where sun: 0, mon: 1, tue: 2, wed: 3, thu: 4, fri: 5, sat: 6 (so margin-left: $index * ($cell-size + $gap) positions the first day at the correct column for each modifier).

Comment on lines 60 to 64
.calendar__day:hover {
cursor: pointer;
background-color: #ffbfcb;
transform: translateY(-20px);
transition-duration: 0.5s;

Choose a reason for hiding this comment

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

Transition is declared only in the :hover rule which can prevent smooth animation when the element returns to its normal state. Move the transition to the base .calendar__day (e.g. transition: background-color 0.5s, transform 0.5s;) so both hover-in and hover-out animate for the required 0.5s duration.

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

Excellent work on addressing the previous feedback! You've correctly implemented all the required SCSS logic in the new src/styles/main.scss file, including the fixes for the weekday mapping and the hover transition.

However, there is one small but critical issue remaining. The src/index.html file is still linking to the old styles/index.scss stylesheet, which prevents all of your new, correct styles from being applied.

Please update the <link> tag's href attribute in src/index.html to point to styles/main.scss. Once you make that one change, 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

src/index.html Outdated
@@ -12,7 +13,40 @@
href="styles/index.scss"

Choose a reason for hiding this comment

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

The task requires linking to src/styles/main.scss. You've created the correct stylesheet with the fixes, but this HTML file still links to the old styles/index.scss. Please update the href attribute to styles/main.scss.

Comment on lines 4 to 12
$weekdays: (
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 mapping for $weekdays is unconventional. For a standard calendar layout where the week starts on Sunday, the index for sun should be 0, mon should be 1, and so on. It looks like you've already made this correction in the main.scss file, which is excellent! The next step is to make sure the HTML uses that corrected file.

cursor: pointer;
background-color: #ffbfcb;
transform: translateY(-20px);
transition-duration: 0.5s;

Choose a reason for hiding this comment

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

Placing the transition-duration here means the animation only applies when you hover onto an element, but not when you move the mouse away. To meet the requirement that all changes are animated, this property should be moved to the base .calendar__day rule. I see you've already corrected this in your main.scss file, which is perfect.

Comment on lines +12 to +14
);

body {

Choose a reason for hiding this comment

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

The task requires styles to be loaded from src/styles/main.scss. You have created the correct file, but this link still points to the old index.scss. Please update the href attribute to "styles/main.scss".

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

Excellent work on this task! You've successfully fixed the stylesheet link, and the calendar now renders and functions perfectly. I am approving your solution so you can move forward, but I have one small suggestion for you to consider for future projects. In src/styles/main.scss, it's a good practice to use the shorthand transition property instead of just transition-duration. For example, you could use transition: all 0.5s; to make it clear which properties should be animated. This is just a minor style point, and overall your solution is fantastic. Keep up the great work!


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

Footnotes

  1. Rate AI review example

cursor: pointer;
background-color: #ffbfcb;
transform: translateY(-20px);
transition-duration: 0.5s;

Choose a reason for hiding this comment

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

According to the project's code style rules, it's better to use shorthand properties where possible. Please use the transition shorthand property here instead of just transition-duration. For example: transition: all 0.5s;.

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