-
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 task solution #3961
base: master
Are you sure you want to change the base?
add task solution #3961
Conversation
julia303920201
commented
Nov 20, 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.
Hello! 😊 It looks like you're making great progress, but there are a few critical issues that need to be addressed before we can approve the pull request:
-
File Naming and Location: The styles are currently in
src/styles/index.scss
, but they should be insrc/styles/main.scss
as per the task requirements. This is important to ensure consistency and adherence to the project structure. -
Vertical Centering: The calendar needs to be centered both horizontally and vertically on the screen. Currently, it is only centered horizontally. Please update your SCSS to achieve vertical centering as well.
-
Linking Stylesheet: The HTML file is linking directly to a
.scss
file. Make sure to compile your SCSS into a CSS file and link that CSS file instead. This is crucial for the stylesheet to be applied correctly. -
Modifiers Implementation: Ensure that the
start-day
andmonth-length
modifiers are correctly implemented and reflect the intended changes in the calendar layout. -
BEM Methodology: Double-check that all BEM conventions are followed to maintain code readability and organization.
Once these issues are addressed, your solution will be much closer to meeting all the task requirements. Keep up the good work, and don't hesitate to reach out if you need any help! 🚀
✨ 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,39 @@ | |||
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. Typically, .scss
files are not directly used in HTML as they need to be compiled into .css
files. Ensure that the Sass file is compiled into a CSS file and link the resulting CSS file here.
src/styles/index.scss
Outdated
$daySize: 100px; | ||
$countDays: 31; | ||
$gapValue: 1px; | ||
$calendarWidth: $daySize * 7 + 10; |
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 $calendarWidth
does not account for the gaps between the days. Since there are 6 gaps in a 7-day week, consider updating the calculation to $calendarWidth: $daySize * 7 + $gapValue * 6;
to ensure the width includes these gaps.
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 making progress with your project! 🎉 While there are a few issues that need attention, such as renaming index.scss
to main.scss
and ensuring the HTML links to the compiled CSS file instead of the .scss
file, your efforts are commendable. These adjustments are critical for the correct application of styles and adherence to project requirements.
The decision to approve is made to help you move forward with your learning journey. Please review the comments and address these issues to enhance your code quality. Remember, learning from these experiences is key to your growth as a developer. Keep up the good work and continue improving! 💪
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
@@ -12,7 +12,39 @@ | |||
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 HTML file is linking to a .scss
file. You should compile your SCSS into a CSS file and link to that CSS file instead. This is crucial for the stylesheet to be applied correctly.
@@ -1,3 +1,85 @@ | |||
$day-size: 100px; |
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 file is named index.scss
, but according to the task requirements, it should be named main.scss
. Please rename the file to ensure consistency with the project structure.