-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Develop #3926
base: master
Are you sure you want to change the base?
Develop #3926
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.
Great job on implementing the calendar! 🎉 While there are a few minor issues, such as linking to a SCSS file instead of a compiled CSS file, these do not critically impact the functionality. Keep up the good work and consider these points for future improvements. Happy coding! 😊
✨ 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 href
attribute is pointing to a SCSS file (styles/main.scss
). Browsers cannot directly interpret SCSS files. You need to compile the SCSS into a CSS file and link to that CSS file instead.
@@ -0,0 +1,9 @@ | |||
body { | |||
justify-content: center; |
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.
Using justify-content
on the body element requires the body to be a flex container, which is correctly set on line 7. However, ensure that the body has a defined height or width to see the centering effect.
@@ -0,0 +1,9 @@ | |||
body { | |||
justify-content: center; | |||
align-items: center; |
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.
Using align-items
on the body element requires the body to be a flex container, which is correctly set on line 7. Ensure that the body has a defined height or width to see the centering effect.
gap: 1px; | ||
padding: 10px; | ||
display: flex; | ||
width: ($widthDays + $gapDays) * 7; |
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 the calendar width ($widthDays + $gapDays) * 7
assumes that the gap is included in the width of each day. Ensure that this calculation aligns with your design expectations, as the gap might need to be subtracted if it's not part of the width.
reflects in changing calendar layout
Typical Mistakes
fromBEM
lesson theory are checked.