Develop #5615
Develop #5615
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 While you've done a great job with the structure and styling, there are a few critical issues that need to be addressed before we can approve the pull request. The use of an absolute path for the logo image and the unencoded ampersand in the href attribute are significant as they can impact the functionality and validity of the HTML. Please make these changes, and you'll be all set! Keep up the great work! 🚀
✨ 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
| </li> | ||
| <li class="nav_item"> | ||
| <a | ||
| href="#laptops&computers" |
There was a problem hiding this comment.
The href attribute contains an ampersand (&) which should be encoded as & to ensure valid HTML.
src/index.html
Outdated
| <div class="logo"> | ||
| <a href="index.html"> | ||
| <img | ||
| src="/src/images/logo.png" |
There was a problem hiding this comment.
Consider using a relative path for the src attribute of the logo image to avoid issues if the file structure changes or if the site is deployed to a different environment.
src/style.css
Outdated
|
|
||
| ul { | ||
| margin: 0; | ||
| margin-block: 0; |
There was a problem hiding this comment.
The margin-block: 0; is redundant since margin: 0; already sets all margins to zero. Consider removing it to simplify the code.
src/style.css
Outdated
| ul { | ||
| margin: 0; | ||
| margin-block: 0; | ||
| margin-inline: 0; |
There was a problem hiding this comment.
The margin-inline: 0; is redundant since margin: 0; already sets all margins to zero. Consider removing it to simplify the code.
There was a problem hiding this comment.
Great job on your submission! 🎉 Your code meets most of the task requirements and constraints, which is a solid foundation. However, there are a few areas for improvement:
- CSS Selector Usage: Avoid using the universal selector
*as it can impact performance. Focus on setting styles only for necessary elements. - Image Alt Attribute: The
altattribute for the logo image should be more descriptive to improve accessibility. - Dynamic Active Link: Ensure the
is-activeclass is applied dynamically to reflect the current active link accurately. - Data Attributes: Verify the necessity and correct usage of the
data-qaattribute, typically used for testing purposes.
These adjustments will enhance your code's performance and maintainability. Keep up the great work, and use this feedback to refine your skills further. Remember, addressing these issues will help you avoid similar problems in the future. Happy coding! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <li class="nav_item"> | ||
| <a | ||
| href="#apple" | ||
| class="nav_link is-active" |
There was a problem hiding this comment.
The 'is-active' class is applied to the 'apple' link. Ensure this is intentional and consider making it dynamic if the active state should change based on the current section or page.
| <a | ||
| href="#laptops-and-computers" | ||
| class="nav_link" | ||
| data-qa="hover" |
There was a problem hiding this comment.
The 'data-qa' attribute is used on the 'laptops & computers' link. Ensure this attribute is necessary and used correctly, typically for testing purposes.
| .nav_link.is-active { | ||
| color: var(--main-lc); | ||
| } |
There was a problem hiding this comment.
The '.nav_link.is-active' class changes the color of the active link. Ensure this class is applied dynamically to the currently active link in the HTML to reflect the active state correctly.
| .nav_link[data-qa='hover']:hover { | ||
| color: var(--main-lc); | ||
| } |
There was a problem hiding this comment.
The '.nav_link[data-qa='hover']:hover' selector changes the color on hover. Ensure the 'data-qa' attribute is used correctly, typically for testing purposes, and verify its necessity.
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 ❗️