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 #4832

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

Conversation

GrzegorzLasota
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.


It looks like you're almost there! However, there's a critical issue that needs to be addressed before we can approve the pull request. The HTML file is currently linking directly to a .scss file, which is not standard practice. SASS files need to be compiled into CSS before they can be linked in an HTML document. Please ensure that styles/main.scss is compiled into styles/main.css and update the link in your HTML file accordingly. This change is essential for the correct functionality of your project. Once this is fixed, your work will be ready for approval! Keep up the great effort! 😊

✨ 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.

@@ -17,11 +17,439 @@
/>
<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 stylesheet link references a '.scss' file. Typically, SASS files need to be compiled into CSS before being used in HTML. Ensure that 'styles/main.scss' is compiled to 'styles/main.css' and update the href attribute accordingly.

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.

Review Summary

Great job on your submission! 🎉 Your code meets most of the task requirements and constraints, and it's clear that you've put a lot of effort into organizing your styles and using modern CSS features effectively. Here are some key points and suggestions for improvement:

  1. BEM and SCSS Usage: Your SCSS files seem to follow BEM conventions, which is excellent. Ensure that all components consistently use BEM naming conventions throughout the code.

  2. Data Attributes: Remember to add data-qa="nav-hover" to the 4th nav link (Laptops & computers) as specified in the task.

  3. Hover Effects: Ensure all hover effects are smooth and implement the specified changes, such as changing the navigation link text color to #00acdc.

  4. Color Variables: Consider defining repeated sizes and special colors as variables in your SCSS files to enhance maintainability and readability.

  5. HTML Structure: You've correctly used the <main> tag for the cards container, which is great!

  6. Stylesheet Link: The href attribute in your HTML is pointing to a .scss file. Ensure this file is being preprocessed into CSS, as browsers cannot interpret SASS files directly. If not, change the link to point to the compiled CSS file.

Your work is approved to help you move forward with your learning journey. Please review the comments to improve your code. Addressing these issues will help you avoid similar problems in the future and enhance your coding skills. Keep up the great work and continue learning from your experiences! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

@@ -17,11 +17,439 @@
/>
<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 pointing to a .scss file. Ensure that this file is being preprocessed into CSS, as browsers cannot directly interpret SASS files. If not, change the link to point to the compiled CSS file.

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