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

Resolve the task #5953

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

Conversation

maryna-filipova
Copy link

  • DEMO LINK

  • TEST REPORT LINK

  • Header height is set in 1 place (for the links)

  • Content is vertically centered (for any header height)

  • CSS is used to show all letters in Uppercase (don't type them in HTML)

  • Logo is an image wrapped with a link

  • CSS Variable is used for a blue color

  • Pseudo-element is used for a blue line below the active link

  • Code follows all the Code Style Rules ❗️

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 effort on your submission! 🎉 While your code is generally well-structured, there are a few areas for improvement to fully meet the task requirements:

  1. CSS Universal Selector: Avoid using the universal selector * for resetting margins and paddings as it can impact performance.

  2. Semantic Tags: Enhance the alt attribute for images to be more descriptive than just "logo".

  3. CSS Variable for Blue Color: Implement a CSS variable for the blue color as specified in the task.

  4. CSS Flex Gap Property: Use margin instead of the gap property for indents as per the task requirements.

  5. HTML Formatting: Ensure consistent use of 2 spaces for indentation instead of mixing spaces and tabs.

  6. CSS for Uppercase Letters: Use CSS to transform text to uppercase rather than writing it directly in uppercase in HTML.

  7. Navigation Link: The href attribute value #laptops&computers should be encoded as #laptops%26computers to avoid issues.

  8. Redundant Styles: Address redundancy in padding styles for the header element and conflicting margin styles on the nav element.

These issues should be addressed to enhance your code's performance and maintainability. The decision to approve is made to help you progress in your learning journey. Please review the comments and make improvements in your future submissions. 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! ✨

<li><a href="#smartphone">SMARTPHONES</a></li>
<li>
<a
href="#laptops&computers"

Choose a reason for hiding this comment

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

The 'href' attribute value '#laptops&computers' may cause issues because the '&' character is not encoded. Consider replacing it with '%26' to ensure proper URL encoding: '#laptops%26computers'.

Comment on lines +17 to +18
padding: 0 20px;
padding-left: 40px;

Choose a reason for hiding this comment

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

There is a redundancy in the padding styles for the 'header' element. You have both 'padding: 0 20px;' and 'padding-left: 40px;'. Consider removing 'padding: 0 20px;' to avoid confusion and ensure the intended padding is applied.

Comment on lines +28 to +30
margin-left: auto;
right: 40px;
margin-right: 50px;

Choose a reason for hiding this comment

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

The 'margin-left: auto;' and 'margin-right: 50px;' styles on the 'nav' element might conflict with each other. If the intention is to align the navigation to the right, consider using only 'margin-left: auto;'.

@ITguy-star
Copy link

HI @maryna-filipova I cannot pass this task it's give me 2 errors, can you help me?
Moyo

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.

3 participants