Skip to content

init#5200

Open
Peloso31 wants to merge 3 commits intomate-academy:masterfrom
Peloso31:master
Open

init#5200
Peloso31 wants to merge 3 commits intomate-academy:masterfrom
Peloso31:master

Conversation

@Peloso31
Copy link

No description provided.

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 issues that block full task completion and correct behavior.

Critical blockers (must fix):

  1. Stylesheet path/filename mismatch: The task explicitly requires styles in src/styles/main.scss, but your SCSS is in src/styles/index.scss and the HTML links to styles/index.scss. This breaks the expected project structure and automated checks. Please move/rename the file to src/styles/main.scss and update the <link> in src/index.html to point to styles/main.css (or the compiled equivalent).
  2. Incorrect start-day offsets: In src/styles/index.scss, your $start-days map currently treats Monday as the leftmost column ('mon': 0'sun': 6). The requirement is that the calendar starts from Sunday by default, and changing the start-day modifier should place the 1st on the correct column. Adjust the map so that Sunday is offset 0 (e.g. 'sun': 0, 'mon': 1, ... 'sat': 6) or otherwise ensure the margin-left calculation aligns with a Sunday-first layout; then verify that changing calendar--start-day-* actually shifts the first day correctly. This directly affects [CHECKLIST ITEM #1].
  3. Cursor behavior on hover: You currently set cursor: pointer; on .calendar__day at all times (around line 39 in src/styles/index.scss). The requirement says the cursor should become pointer on hover. Move cursor: pointer; into the .calendar__day:hover (or &:hover) block so the cursor only changes when hovering a cell.

You’ve done a solid job on most SCSS features (variables, @for/@each, ::before numbering, flex layout, hover animation), and your HTML has the right BEM structure and modifiers. Once you fix the file name/path, correct the start-day mapping, and adjust the cursor rule, your solution should fully meet the functional requirements and pass the 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

@@ -1,3 +1,84 @@
// Variables

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 stylesheet is named src/styles/index.scss. Move/rename the file to src/styles/main.scss so it matches the required project structure and automatic checks.

Comment on lines 61 to 68
$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.

The $start-days map maps 'mon' -> 0 and 'sun' -> 6 which assumes Monday is the leftmost column. The task expects the calendar to start from Sunday by default and that start-day modifiers place the 1st on the correct column. This will produce incorrect month starts and breaks: "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout" [CHECKLIST ITEM #1]. Consider changing the offsets so 'sun': 0, 'mon': 1, 'tue': 2, 'wed': 3, 'thu': 4, 'fri': 5, 'sat': 6 (or adjust according to your chosen leftmost column), then the margin-left calculation will place first day correctly.

display: flex;
justify-content: center;
align-items: center;
cursor: pointer;

Choose a reason for hiding this comment

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

You set cursor: pointer; on .calendar__day at all times (line 39). The requirement states: "On hovering over a cell: cursor should become pointer." Move cursor: pointer; into the &:hover block so the cursor becomes pointer only on hover.

}

// month-length modifiers — @for to hide days beyond the chosen length
@for $i from 28 through 31 {

Choose a reason for hiding this comment

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

I don't see the HTML (src/index.html) provided with this submission. Without it I cannot verify HTML-dependent requirements: "Create a markup for the calendar block with 31 days inside", "Set calendar to start from Sunday by default in your HTML", and checklist item #2: "Each day has no modifiers, only class (eg. calendar__day)". Please add src/index.html with the calendar markup (31 .calendar__day elements and default start-day modifier set to Sunday) so I can finish verification.

</head>
<body>
<h1>Calendar</h1>
<div class="calendar calendar--start-day-sun calendar--month-length-31">

Choose a reason for hiding this comment

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

The HTML currently links to styles/index.scss (in the ) and the project requires styles written in src/styles/main.scss. Please place your SCSS in src/styles/main.scss and upload that file so I can validate SCSS-specific requirements (use of @for/@each, ::before numbering, flex layout, hover behavior, nth-child month-length logic, etc.). Without the SCSS file I cannot verify checklist items that depend on styling, including: "Changing 'month-lengh' and 'start-day' modifier in the code element reflects in changing calendar layout" [CHECKLIST ITEM #1], "All Typical Mistakes from BEM lesson theory are checked." [CHECKLIST ITEM #3], and "Code follows all the Code Style Rules ❗️" [CHECKLIST ITEM #4].

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 is still a critical functional issue with the start-day logic that breaks the main requirement of correctly positioning the first day of the month. Right now, in src/styles/main.scss (lines ~62–69 and the @each block around 72–77), the $start-days map treats sun as offset 0 and mon as 1, which produces a Sunday-first layout. The task explicitly requires a Monday-first calendar where Monday is the first column, Friday is the fifth, and the HTML default modifier calendar--start-day-sun must place Sunday in the last column, not the first [CHECKLIST ITEM #1].

To fix this, you need to update the offset logic so that Monday maps to 0, Tuesday to 1, …, Sunday to 6, or equivalently adjust the margin-left calculation so that calendar--start-day-mon places day 1 in the leftmost column and calendar--start-day-sun moves it to the rightmost column. Once this map and/or calculation is corrected, changing .calendar--start-day-* modifiers should visually move the first day into the correct weekday column in all scenarios. Everything else in your implementation (HTML structure, day count, month-length modifiers, hover behavior, sizing, and flex layout) looks solid, so you only need to focus on correcting this offset mapping. After you adjust the offsets and verify that the Backstop tests for different start-day variants pass, your solution should fully meet the task 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 62 to 76
$start-days: (
'sun': 0,
'mon': 1,
'tue': 2,
'wed': 3,
'thu': 4,
'fri': 5,
'sat': 6,
);

@each $day, $offset in $start-days {
.calendar--start-day-#{$day} .calendar__day:first-child {
@if $offset > 0 {
margin-left: $offset * $day-step;
}

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 treats 'sun' as offset 0 (leftmost column) and 'mon' as offset 1 (lines 62–69 and the @each block 72–77). The task requires a Monday-first week (Monday should be column 1, Friday column 5), and the HTML default must be calendar--start-day-sun (so 'sun' should map to the last column). Update the offsets so Monday maps to 0, Tuesday 1, … Sunday 6 (or otherwise adjust the margin-left calculation to reflect a Monday-first layout). After that, changing .calendar--start-day-* classes will place day 1 in the correct 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

Your solution is approved — it correctly fulfills the calendar task requirements, including BEM structure, 31 day elements in the HTML, Sass-generated numbers via ::before, and the start-day and month-length modifiers in src/styles/main.scss. The layout, sizing, spacing, and hover animation all match the specification: 100px square days with 1px borders, flex layout with a 1px gap, and smooth hover transitions. Your use of Sass features like @for, @each, variables, and maps is clear and well-structured, which will be very helpful as projects get more complex.

This is an educational override to help you progress while learning: there are no blocking issues, but there are a couple of optional improvements you could explore. For accessibility, consider adding semantic/ARIA roles in index.html (for example, role="grid" on the main calendar container and role="gridcell" on each day) so screen readers can better interpret the calendar structure. As a tiny robustness polish, setting display: block; on the ::before pseudo-element that renders the numbers can make centering behavior more consistent across different browsers. Overall, you showed solid command of both HTML structure and SCSS, and you’re very close to production-quality code here — keep building on this foundation.


✨ 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