add solution #5266
add solution #5266
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 However, there are a few critical issues that need to be addressed before we can approve the pull request. The main concerns are the typos in the class name and import statement for 'container', which can lead to styling issues. Additionally, the use of display: flex; on an image element is unconventional and should be reconsidered unless necessary. Please make these corrections to ensure the functionality and requirements of the task are fully met. 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.
src/index.html
Outdated
| <div | ||
| class="conteiner" |
There was a problem hiding this comment.
There is a typo in the class name 'conteiner'. It should be 'container'. This typo may cause issues with styling if the CSS is expecting 'container'.
src/styles/blocks/conteiner.scss
Outdated
| @@ -0,0 +1,3 @@ | |||
| .conteiner { | |||
There was a problem hiding this comment.
There is a typo in the class name '.conteiner'. It should be '.container' to match the corrected class name in the HTML file.
src/styles/blocks/header.scss
Outdated
| @@ -0,0 +1,8 @@ | |||
| .header__img { | |||
| margin: 0 auto; | |||
| display: flex; | |||
There was a problem hiding this comment.
Using 'display: flex;' on an image element is unusual unless you have a specific reason to apply flexbox properties to an image. Typically, 'display: block;' or 'display: inline-block;' is used for images. Consider revising this if flexbox behavior is not required.
src/styles/index.scss
Outdated
| @import 'blocks/menu'; | ||
| @import 'blocks/header'; | ||
| @import 'blocks/footer'; | ||
| @import 'blocks/conteiner'; |
There was a problem hiding this comment.
There is a typo in the import statement 'blocks/conteiner'. It should be 'blocks/container' to match the corrected file name and ensure the styles are imported correctly.
There was a problem hiding this comment.
Your Pull Request has been approved! 🎉 While there are some areas for improvement, this decision is made to help you progress in your learning journey. Please review the comments and address the following key points to enhance your code:
- SCSS Compilation: Ensure your build process correctly compiles SCSS to CSS. Directly linking SCSS files in HTML is not standard unless using a tool like Parcel.
- Flexbox Properties: The
align-content: center;andalign-items: center;properties require the parent element to be a flex container. Verify thatdisplay: flex;is set where needed. - SCSS Variables: Consider creating a separate file for SCSS variables to improve maintainability and meet task requirements.
These adjustments will help you avoid similar issues in the future. Keep up the great work, and continue learning from these experiences! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <link | ||
| href="https://fonts.googleapis.com/css2?family=Poppins:ital,wght@0,100;0,200;0,300;0,400;0,500;0,600;0,700;0,800;0,900;1,100;1,200;1,300;1,400;1,500;1,600;1,700;1,800;1,900&family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&display=swap" | ||
| rel="stylesheet" | ||
| /> | ||
| <link | ||
| rel="stylesheet" | ||
| href="./styles/index.scss" |
There was a problem hiding this comment.
Ensure that your build process correctly compiles SCSS to CSS. Linking SCSS files directly in HTML is only valid if you are using a tool like Parcel that handles SCSS compilation.
| line-height: 16px; | ||
| text-transform: uppercase; | ||
| text-align: center; | ||
| align-items: center; |
There was a problem hiding this comment.
The align-items: center; property will only have an effect if the button is a flex container. Ensure that the button or its parent is set to display: flex; if you intend to use this property.
| line-height: 19px; | ||
| text-align: right; | ||
| color: #060b35; | ||
| align-content: center; |
There was a problem hiding this comment.
The align-content: center; property will only have an effect if the element is a flex container. Ensure that the parent element is set to display: flex; if you intend to use this property.
DEMO LINK
TEST REPORT LINK