Skip to content
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 #3923

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

leraMeleshchenko
Copy link

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a 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:

  1. SCSS File Location and Naming: The task specifies that styles should be written in src/styles/main.scss, but your styles are currently in src/styles/index.scss. Please rename and relocate the file to meet the task requirements.

  2. CSS Pseudo-element Usage: The task requires using ::before to add numbers in the center of each day, but your implementation uses :after. This needs to be corrected to align with the task specifications.

  3. Selector Issues:

    • The selector .calendar--start-day-#{$day} :first-child has a space before :first-child, making it a descendant combinator. This may not work as intended. Please remove the space to target the first child directly.
    • Similarly, the selector .calendar :nth-child(#{$i}):after has a space before :nth-child, which may cause it to select unintended elements. Please adjust this to ensure it targets the correct elements.

These changes are crucial to ensure the functionality and correctness of the calendar layout. Once these issues are resolved, 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.

Comment on lines 39 to 43
.calendar :nth-child(#{$i}):after {
content: '#{$i}';
font-family: Arial, Helvetica, sans-serif;
font-size: 30px;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selector .calendar :nth-child(#{$i}):after may not work as intended because the space before :nth-child makes it a descendant combinator. This means it will select any element that is the nth child of its parent, not necessarily the direct children of .calendar. Consider removing the space to target direct children.

Comment on lines 57 to 59
.calendar--start-day-#{$day} :first-child {
margin-left: ($day-size + $gap) * $index;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selector .calendar--start-day-#{$day} :first-child may not work as intended due to the space before :first-child, which makes it a descendant combinator. This will select any first child of any element within .calendar--start-day-#{$day}. Consider removing the space to target the first child of .calendar--start-day-#{$day} directly.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job on completing the task! 🎉 Your code meets the task requirements and effectively uses SCSS and BEM methodologies. However, there are a few critical issues to address:

  1. Stylesheet Link: The HTML file links to a SCSS file instead of a compiled CSS file. Ensure you link to the correct CSS file to avoid rendering issues.

  2. Selector Specificity: The selectors .calendar--start-day-#{$day} :first-child and .calendar :nth-child(#{$i})::before might be too broad. Consider narrowing them to ensure they target the intended elements.

  3. Transition Optimization: Specify transition-property to improve performance by limiting the animated properties.

These improvements will enhance your code's functionality and maintainability. 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! ✨

@@ -9,10 +9,42 @@
<title>Calendar</title>
<link
rel="stylesheet"
href="styles/index.scss"
href="styles/main.scss"

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/main.scss). SCSS files need to be compiled into CSS before they can be used in an HTML document. Make sure to link to the compiled CSS file instead.

background-color: #ffbfcb;
transform: translateY(-20px);
cursor: pointer;
transition-duration: 0.5s;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding transition-property to specify which properties should transition. This can improve performance by limiting the properties that are animated.

}

@for $i from 1 to 32 {
.calendar :nth-child(#{$i})::before {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selector .calendar :nth-child(#{$i})::before might be too broad, as it applies to any child element of .calendar. Ensure this is the intended behavior, or consider narrowing the selector if needed.

);

@each $day, $index in $days {
.calendar--start-day-#{$day} :first-child {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selector .calendar--start-day-#{$day} :first-child might be too broad, as it applies to the first child of any element within .calendar--start-day-#{$day}. Ensure this is the intended behavior, or consider narrowing the selector if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants