-
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
calendar #4013
base: master
Are you sure you want to change the base?
calendar #4013
Conversation
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
href
attribute in your HTML is linking directly to an SCSS file. Browsers cannot interpret SCSS files directly, so you need to compile your SCSS into a CSS file and link to that CSS file instead. -
SCSS Variables: Ensure that all SCSS variables such as
$calendar-width
,$gap
,$day-size
,$day-color
,$day-border
,$font-size
, and$weekdays
are defined or imported correctly. Missing definitions can lead to styling issues. -
Month-Length Modifier: The
month-length
modifier is not visible in the provided SCSS code. This needs to be implemented to set the last day to show usingnth-child
.
Once these issues are resolved, your code will meet the task requirements. Keep up the great work, and feel free to reach out if you have any questions! 🚀
✨ 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.
@@ -12,7 +12,41 @@ | |||
href="styles/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 href
attribute is linking to a SCSS file (styles/index.scss
). Browsers cannot directly interpret SCSS files. You need to compile the SCSS into a CSS file and link to the resulting CSS file instead.
.calendar { | ||
display: flex; | ||
flex-wrap: wrap; | ||
width: $calendar-width; |
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 variable $calendar-width
is defined. If it's not defined in this file, it should be imported from another SCSS file.
width: $calendar-width; | ||
padding: 0; | ||
margin: auto; | ||
gap: $gap; |
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 variable $gap
is defined. If it's not defined in this file, it should be imported from another SCSS file.
|
||
&__day { | ||
box-sizing: border-box; | ||
width: $day-size; |
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 variable $day-size
is defined. If it's not defined in this file, it should be imported from another SCSS file.
box-sizing: border-box; | ||
width: $day-size; | ||
height: $day-size; | ||
background-color: $day-color; |
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 variable $day-color
is defined. If it's not defined in this file, it should be imported from another SCSS file.
width: $day-size; | ||
height: $day-size; | ||
background-color: $day-color; | ||
border: $day-border; |
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 variable $day-border
is defined. If it's not defined in this file, it should be imported from another SCSS file.
justify-content: center; | ||
align-items: center; | ||
font-family: Arial, sans-serif; | ||
font-size: $font-size; |
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 variable $font-size
is defined. If it's not defined in this file, it should be imported from another SCSS file.
} | ||
} | ||
|
||
@each $day, $number in $weekdays { |
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 variable $weekdays
is defined. If it's not defined in this file, it should be imported from another SCSS file.
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 constraints, and you've done well to address previous issues. However, there are a few critical points to keep in mind for future improvements:
-
SCSS Compilation: Ensure that your SCSS is compiled into CSS before linking it in your HTML. This is crucial as browsers cannot interpret SCSS files directly.
-
SCSS Variables: While your
variables.scss
file defines necessary variables, make sure they are correctly imported into your main SCSS file to avoid undefined variable issues. -
Code Structure: Your implementation follows the BEM methodology and the HTML structure is well-organized, which is excellent.
Remember, addressing these issues will enhance the functionality and maintainability of your code. Keep up the good work and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -12,7 +12,41 @@ | |||
href="styles/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 href
attribute is linking to an SCSS file (styles/index.scss
). Browsers cannot interpret SCSS files directly. You need to compile your SCSS into a CSS file and link to that CSS file instead.
.calendar { | ||
display: flex; | ||
flex-wrap: wrap; | ||
width: $calendar-width; |
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 variable $calendar-width
is used here but is not defined in this file. Ensure that it is defined or imported from another file.
width: $calendar-width; | ||
padding: 0; | ||
margin: auto; | ||
gap: $gap; |
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 variable $gap
is used here but is not defined in this file. Ensure that it is defined or imported from another file.
|
||
&__day { | ||
box-sizing: border-box; | ||
width: $day-size; |
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 variable $day-size
is used here but is not defined in this file. Ensure that it is defined or imported from another file.
box-sizing: border-box; | ||
width: $day-size; | ||
height: $day-size; | ||
background-color: $day-color; |
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 variable $day-color
is used here but is not defined in this file. Ensure that it is defined or imported from another file.
width: $day-size; | ||
height: $day-size; | ||
background-color: $day-color; | ||
border: $day-border; |
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 variable $day-border
is used here but is not defined in this file. Ensure that it is defined or imported from another file.
justify-content: center; | ||
align-items: center; | ||
font-family: Arial, sans-serif; | ||
font-size: $font-size; |
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 variable $font-size
is used here but is not defined in this file. Ensure that it is defined or imported from another file.
} | ||
} | ||
|
||
@each $day, $number in $weekdays { |
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 variable $weekdays
is used here but is not defined in this file. Ensure that it is defined or imported from another file.
❗️ Copy this
Checklist
to thePull Request
description after links, and put- [x]
before each point after you checked it.reflects in changing calendar layout
Typical Mistakes
fromBEM
lesson theory are checked.