add homework #3979
add homework #3979
Conversation
anastvozniuk
commented
Nov 28, 2024
anastvozniuk
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
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:
-
File Naming and Location: The SCSS file should be named
main.scssand located insrc/styles/as per the task requirements. Please rename the file and update its location. -
HTML Link Update: The HTML file currently links to
styles/index.scssand./../css/index.css. Once the SCSS file is renamed tomain.scss, ensure the HTML file is updated to link correctly to this file. -
Vertical Centering: The calendar needs to be centered vertically on the screen. Please add the necessary styles to achieve this.
These changes are crucial for meeting the core requirements of the task. Once these are addressed, your code will be in great shape! Keep up the great 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.
css/index.css
Outdated
| display: -webkit-box; | ||
| display: -ms-flexbox; | ||
| display: flex; |
There was a problem hiding this comment.
Consider removing vendor prefixes for display: flex; as modern browsers have good support for flexbox without prefixes. Use tools like Autoprefixer if you need to support older browsers.
css/index.css
Outdated
| display: -webkit-box; | ||
| display: -ms-flexbox; | ||
| display: flex; |
There was a problem hiding this comment.
Similar to the previous comment, check if vendor prefixes for display: flex; are necessary. Modern browsers typically do not require these prefixes.
css/index.css
Outdated
| -webkit-transition: linear 0.5s; | ||
| transition: linear 0.5s; |
There was a problem hiding this comment.
Consider removing the -webkit-transition prefix unless you need to support very old versions of WebKit browsers. Modern browsers support the unprefixed transition property.
css/index.css
Outdated
| -webkit-transform: translateY(-20px); | ||
| -ms-transform: translateY(-20px); | ||
| transform: translateY(-20px); |
There was a problem hiding this comment.
Check if vendor prefixes for transform: translateY(-20px); are necessary. Most modern browsers support the unprefixed version.
src/index.html
Outdated
| <link | ||
| rel="stylesheet" | ||
| href="./../css/index.css" |
There was a problem hiding this comment.
The link to the stylesheet is pointing to ./../css/index.css. Ensure that this path is correct relative to the HTML file's location. If the file structure is different, update the path accordingly.
src/styles/index.scss
Outdated
| &:first-child { | ||
| margin-left: 606px; |
There was a problem hiding this comment.
The margin-left value is hardcoded as 606px. Consider calculating this value using the existing SCSS variables to maintain consistency and flexibility. For example, you could use $columns-size * 6 or another appropriate calculation based on your layout requirements.
There was a problem hiding this comment.
Great job on addressing the task requirements! 🎉 Your code is well-structured, and you've effectively used SCSS features like loops and mixins to enhance maintainability and scalability. The calendar is centered using flexbox, which is a modern and efficient approach.
However, there's a critical issue with the HTML file linking to the SCSS file instead of the compiled CSS file. Please update the href attribute to link to css/main.css to ensure styles are applied correctly in the browser. This is crucial for the functionality of your project.
Keep up the good work, and make sure to address this issue to avoid similar problems in the future. Your progress is impressive, and with these adjustments, your project will be even better! 🚀
✨ 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.
The href attribute currently links to styles/main.scss. This should be updated to link to the compiled CSS file css/main.css to ensure the styles are applied correctly in the browser.
| body { | ||
| margin: 0; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| height: 100vh; |
There was a problem hiding this comment.
The body styles are correctly set to center the content vertically and horizontally using flexbox, which aligns with the task requirement to center the calendar on the screen.
| @each $day, $margin in (mon: 1, tue: 2, wed: 3, thu: 4, fri: 5, sat: 6) { | ||
| &.calendar--start-day-#{$day} { | ||
| .calendar__day:first-child { | ||
| margin-left: calc((#{$day-size} + #{$gap}) * (#{$margin - 1})); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The use of the @each loop to dynamically generate classes for different starting days of the week is a good practice, making the code more maintainable and scalable.
| @for $i from 28 through 31 { | ||
| &.calendar--month-length-#{$i} { | ||
| .calendar__day:nth-child(n + #{$i + 1}) { | ||
| display: none; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The @for loop is used effectively to handle different month lengths, ensuring that extra days are hidden for months with fewer days.
