stable#5471
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because a few critical requirements are not yet met and they affect core functionality. First, the HTML currently links styles/main.css, while the task explicitly requires linking the SCSS entry (styles/main.scss) so Parcel can compile it; this needs to be updated in src/index.html (the <link> tag) to satisfy the SCSS usage requirement. Second, the default month-length modifier on your <main> element is calendar--month-length-30, but the specification says the calendar must show 31 days by default, so this should be changed to calendar--month-length-31 to align with the task and [CHECKLIST ITEM #1]. Third, the compiled main.css has incorrect selectors such as .calendar--month-length-28__day:nth-child(n+29) instead of .calendar--month-length-28 .calendar__day:nth-child(n+29), meaning changing the month-length modifier in HTML does not actually hide extra days; please ensure your CSS is regenerated from main.scss so the selectors follow the proper BEM structure and modifiers affect the layout as intended. Finally, there are hardcoded values like width: 726px left in main.css, which conflict with your SCSS variable-based calculations; after fixing the linkage and regenerating CSS, these magic numbers should be replaced by the values computed from SCSS variables. Once you fix the SCSS link, set the default to 31 days, and ensure the generated CSS correctly responds to month-length modifiers, your solution will match the functional and checklist requirements and will be in good shape overall.
✨ 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
| <link rel="stylesheet" href="styles/main.css"> | ||
| </head> | ||
| <body class="page-container"> | ||
| <main class="calendar calendar--start-day-sun calendar--month-length-30"> |
There was a problem hiding this comment.
The default month-length modifier here is calendar--month-length-30, but the description requires: "Set 31 days by default in your HTML" and "Set 31 days by default in your HTML" for the month-length modifier, so this should use the 31-days modifier instead.
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>Centered Calendar</title> | ||
| <link rel="stylesheet" href="styles/main.css"> |
There was a problem hiding this comment.
You currently link styles/main.css, but the instructions specifically mention that in this task you can (and should) link *.scss directly; also they say to write styles in src/styles/main.scss. Consider linking the SCSS file so Parcel can bundle it as intended.
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> | ||
| <div class="calendar__day"></div> |
There was a problem hiding this comment.
The fixed width 726px is a leftover hardcoded value that does not match your SCSS variable-based $calendar-width. Since the task asks not to hardcode repeated px values and to use SCSS variables, this compiled CSS should be regenerated from main.scss so the width comes from the calculated variable instead of this magic number.
| min-height: 100vh; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; |
There was a problem hiding this comment.
Per the description, you should link the SCSS file directly (Parcel will bundle it). Using href="styles/main.css" violates the requirement to write and use src/styles/main.scss as the main stylesheet. Consider switching this to the SCSS entry file.
| align-items: center; | ||
| box-sizing: border-box; | ||
| background-color: #f9f9f9; | ||
| } |
There was a problem hiding this comment.
The task requires "Set 31 days by default in your HTML" and the default month-length modifier should also be 31. Using calendar--month-length-30 here contradicts that requirement and means your default layout doesn’t show a full 31-day month.
| display: none; | ||
| } | ||
|
|
||
| .calendar--month-length-29__day:nth-child(n+30) { |
There was a problem hiding this comment.
Same issue as above for month-length-29: .calendar--month-length-29__day:nth-child(n+30) will never match your markup, breaking the required behavior when changing the month-length modifier.
| display: none; | ||
| } | ||
|
|
||
| .calendar--month-length-30__day:nth-child(n+31) { |
There was a problem hiding this comment.
Same issue for month-length-30: the selector should target .calendar__day elements under a calendar with calendar--month-length-30, not a non-existent .calendar--month-length-30__day class.
| display: none; | ||
| } | ||
|
|
||
| .calendar--month-length-31__day:nth-child(n+32) { |
There was a problem hiding this comment.
Same issue for month-length-31: this selector will not respond to changing the calendar--month-length-31 modifier, so the layout won’t update as specified in checklist item #1.
| $border-color: #000; | ||
| $border-width: 1px; | ||
| $grid-gap: 1px; | ||
| $padding: 10px; |
There was a problem hiding this comment.
The task description and note specify that you should link a *.scss file directly (e.g., styles/main.scss) so Parcel can bundle it; pointing this link to main.css violates that requirement and can desynchronize SCSS and built CSS.
| $padding: 10px; | ||
| $columns: 7; | ||
| $transition-duration: 0.5s; | ||
| $hover-lift: -20px; |
There was a problem hiding this comment.
HTML should use calendar--month-length-31 by default (per description: "Set 31 days by default in your HTML"), but here the default is set to calendar--month-length-30, so only 30 days are shown.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because one critical requirement is still not met: the HTML must link the SCSS entry file, not the compiled CSS. The task explicitly says to write styles in src/styles/main.scss and allows linking *.scss directly so Parcel treats main.scss as the source; currently index.html still uses href="styles/main.css", which breaks this requirement and the intended build pipeline. To fix this, update the <link> tag in src/index.html to href="styles/main.scss" so Parcel compiles your SCSS correctly and your calendar styling comes from the SCSS source. Additionally, stylelint reports a minor SCSS formatting issue at src/styles/main.scss:11:72 (scss/operator-no-newline-after); keep the + operator and the next selector on the same line (e.g. selector1 + selector2 { ... }) to satisfy that rule. Functionally and structurally your calendar implementation and SCSS logic look solid, so once you correct the SCSS link and this small linting issue, your solution should fully meet the task requirements. Keep up the good work—these are small but important wiring and style fixes to complete a strong implementation.
✨ 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
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>Centered Calendar</title> | ||
| <link rel="stylesheet" href="styles/main.css"> |
There was a problem hiding this comment.
The task requires: "Write styles in src/styles/main.scss" and explicitly allows linking *.scss files directly in HTML. Linking styles/main.css here violates that requirement and can prevent Parcel from correctly compiling your SCSS entry. Consider changing this href to styles/main.scss so the project uses your SCSS source as intended.
| min-height: 100vh; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; |
There was a problem hiding this comment.
This violates the task requirement: "Write styles in src/styles/main.scss instead of src/style.css" and the note that you may link *.scss directly in HTML. You’re still linking styles/main.css here, so Parcel will not treat main.scss as the entry point. Update this to href="styles/main.scss" so the SCSS file is used as the source and compiled correctly.
| $border-color: #000; | ||
| $border-width: 1px; | ||
| $grid-gap: 1px; | ||
| $padding: 10px; |
There was a problem hiding this comment.
This violates the requirement: "Write styles in src/styles/main.scss instead of src/style.css" and the important note about linking SCSS directly. The HTML is still linking the compiled CSS (styles/main.css), but for this task you must link the SCSS entry so Parcel can compile it, e.g. href="styles/main.scss".
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can continue progressing while you learn, even though there are still a couple of wiring and style issues to fix. You did a very solid job on the SCSS calendar itself: you use variables instead of repeated pixel values, @for for day numbers and month-length modifiers, @each for start-day modifiers, proper BEM naming, and the hover animation behaves exactly as required. The main critical issue is that src/index.html still links styles/main.css instead of styles/main.scss, which goes against the requirement to make src/styles/main.scss the single source of truth and to let Parcel compile from the SCSS entry file; update the <link> to href="styles/main.scss" to align with the build setup. There is also a minor linting/style problem in src/styles/main.scss where the + operator has a newline after it (reported at line 11); keeping the + on the same line will satisfy the style rule and make your SCSS cleaner. Overall, your calendar logic and styling are strong, and once you fix the stylesheet link and the small lint issue, your solution will be both correct and well-aligned with the checklist and tooling. Keep up the good work—this is very close to a fully polished implementation.
✨ 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
| <meta charset="UTF-8"> | ||
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | ||
| <title>Centered Calendar</title> | ||
| <link rel="stylesheet" href="styles/main.css"> |
There was a problem hiding this comment.
This still links styles/main.css, but the task requires writing styles in src/styles/main.scss and explicitly allows linking SCSS files directly; you should change this to href="styles/main.scss" so Parcel treats main.scss as the entry and builds your styles correctly (this also addresses the previous HIGH priority feedback about the link tag).
| min-height: 100vh; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; |
There was a problem hiding this comment.
The task explicitly allows and expects you to link the SCSS entry file so Parcel compiles it; using href="styles/main.css" means your main.scss is not the source of truth. Update this to href="styles/main.scss" to satisfy the requirement to write styles in src/styles/main.scss and let Parcel bundle from SCSS.
| $border-color: #000; | ||
| $border-width: 1px; | ||
| $grid-gap: 1px; | ||
| $padding: 10px; |
There was a problem hiding this comment.
This still links to the compiled CSS file (styles/main.css), but the task explicitly requires writing styles in src/styles/main.scss and allows linking *.scss directly so Parcel treats main.scss as the source. This violates the description requirement to use SCSS as the entry point. Please change this to href="styles/main.scss".
Denys-Kravchuk9988
left a comment
There was a problem hiding this comment.
Demo link doesn't work
https://Volodymyr-Sursayev.github.io/layout_calendar/
|
@Denys-Kravchuk9988 I set up the demo link but now it's leading to the readme.md file. Is it supposed to work this way in this case or should I try get it to connect to index.html? |
reflects in changing calendar layout
Typical MistakesfromBEMlesson theory are checked.