-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add solution #3977
base: master
Are you sure you want to change the base?
add solution #3977
Conversation
Mihakurochkin
commented
Nov 28, 2024
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your hard work on this project! 😊 There are a few critical issues that need to be addressed before we can approve the pull request.
-
SCSS Compilation: The HTML file links directly to an
.scss
file, which browsers cannot interpret. Please compile your SCSS to CSS and update the link accordingly. -
File Structure: The SCSS file is located in
src/styles/blocks/index.scss
, but it should be insrc/styles/main.scss
as per the task requirements. -
Calendar Centering: Ensure the calendar is centered both horizontally and vertically on the screen, as required by the task.
-
Modifiers and Logic: Verify that the
start-day
andmonth-length
modifiers are functioning correctly and reflect changes in the calendar layout. Also, ensure that all variables like$col-count
,$cell-width
,$gap
,$month-length
,$days-of-week
, and$current-day
are correctly defined and used. -
BEM Naming Convention: Double-check that all BEM naming conventions are correctly followed.
Once these issues are resolved, your solution will be much closer to meeting all the task requirements. Keep up the great work, and don't hesitate to reach out if you need further assistance! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review
button in the Reviewers tab.
src/index.html
Outdated
@@ -9,10 +9,42 @@ | |||
<title>Calendar</title> | |||
<link | |||
rel="stylesheet" | |||
href="styles/index.scss" | |||
href="./styles/blocks/index.scss" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stylesheet link references an .scss
file. Browsers cannot directly interpret SCSS files. Ensure that this file is compiled to CSS and link the resulting CSS file instead.
src/styles/blocks/index.scss
Outdated
display: flex; | ||
gap: $gap; | ||
flex-wrap: wrap; | ||
width: $col-count * $cell-width + $gap * $col-count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the variables $col-count
, $cell-width
, and $gap
are defined in the imported variables
file. Otherwise, this calculation will not work as expected.
src/styles/blocks/index.scss
Outdated
border: 1px solid black; | ||
|
||
&:first-child { | ||
margin-left: ($col-count - 1) * $cell-width + $gap * ($col-count - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calculation for margin-left
assumes that $col-count
, $cell-width
, and $gap
are defined and have valid values. Verify these variables are correctly set in the imported file.
src/styles/blocks/index.scss
Outdated
} | ||
|
||
@each $day, $number in $days-of-week { | ||
@if $day == $current-day { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that $days-of-week
and $current-day
are defined and populated correctly. Otherwise, this conditional logic will not function as intended.
src/styles/blocks/index.scss
Outdated
} | ||
} | ||
|
||
@for $i from $month-length + 1 through 32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verify that $month-length
is defined and correctly represents the number of days in the current month. This loop hides days beyond the month's length.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on your submission! 🎉 Your code meets the task requirements, and you've done well in structuring your SCSS and HTML files. However, there are a few critical issues that need attention:
-
SCSS Compilation: Ensure that your SCSS is compiled to CSS, as browsers cannot interpret SCSS files directly. Update your HTML to link to the compiled CSS file.
-
Variable Definitions: Double-check that variables like
$col-count
,$cell-width
,$gap
,$month-length
,$days-of-week
, and$current-day
are correctly defined and used. These are crucial for the layout and functionality of your calendar. -
Logic for Modifiers: Verify that the logic for modifiers such as
start-day
andmonth-length
is functioning correctly. This includes ensuring that$month-length
accurately reflects the number of days in the month and that themargin-left
calculation for&:first-child
accounts for the starting day of the week. -
Vertical Centering: While the calendar is horizontally centered, consider implementing vertical centering for a more polished look.
Your effort is commendable, and addressing these points will enhance your code's functionality and presentation. Keep up the good work, and use this feedback to refine your skills further! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -9,10 +9,42 @@ | |||
<title>Calendar</title> | |||
<link | |||
rel="stylesheet" | |||
href="styles/index.scss" | |||
href="./styles/main.scss" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The HTML file links to a .scss
file, which browsers cannot interpret. Please compile your SCSS to CSS and update the link to point to the compiled CSS file.
display: flex; | ||
gap: $gap; | ||
flex-wrap: wrap; | ||
width: $col-count * $cell-width + $gap * $col-count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that the variables $col-count
, $cell-width
, and $gap
are defined in the imported variables file. This line uses these variables for calculating the width, and they must be correctly set for the layout to work as expected.
border: 1px solid black; | ||
|
||
&:first-child { | ||
margin-left: ($col-count - 1) * $cell-width + $gap * ($col-count - 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The calculation for margin-left
in &:first-child
seems to be incorrect. It should account for the starting day of the week. Verify that the logic aligns with the intended layout.
@each $day, $number in $days-of-week { | ||
@if $day == $current-day { | ||
&:first-child { | ||
margin-left: $number * $cell-width + $gap * $number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure that $days-of-week
and $current-day
are defined and correctly used. This logic is intended to adjust the margin based on the current day, and it must be accurate for the calendar to display correctly.
} | ||
} | ||
|
||
@for $i from $month-length + 1 through 32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The loop for hiding days beyond the $month-length
should be verified. Ensure that $month-length
is set correctly to reflect the actual number of days in the month.