Dev #6109
Dev #6109
Conversation
DrDakka
commented
Feb 10, 2025
DrDakka
commented
- 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 ❗️
- The Google Fonts Configuration follows requirements.
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 While you've made a great start, there are a few critical issues that need to be addressed before we can approve the pull request:
-
CSS Reset: The use of the universal selector
*for resetting margins and paddings can significantly impact performance. Consider using a more targeted approach. -
Font Configuration: The task requires using only the Roboto font with specific settings, but the current CSS includes
serifas a fallback. Please adjust this to meet the task requirements. -
Semantic Tags: Ensure that all required semantic tags are used appropriately in the HTML structure as specified in the task description.
-
Header Height: The header height should be set for the nav links, not the header itself. This needs to be corrected to align with the task requirements.
-
Flexbox Usage: The task specifies not to use the
flexgap property for indents. Instead, usemarginto achieve the desired spacing.
These issues are important to resolve as they impact the core requirements of the task. Once these are addressed, your code will be in great shape! Keep up the good work, and feel free to reach out if you have any questions or need further guidance. 🌟
✨ 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.
| <a | ||
| class="logo" | ||
| href="index.html" |
There was a problem hiding this comment.
Consider adding aria-label or title attribute to the logo link for better accessibility, as it currently only contains an image.
| <a | ||
| class="nav__link is-active" | ||
| href="#" |
There was a problem hiding this comment.
The link's href attribute is set to #, which is generally used as a placeholder. Ensure that this is intentional and consider providing a valid URL or using JavaScript to handle the click event if needed.
src/index.html
Outdated
| <a | ||
| class="nav__link returned" | ||
| href="#" |
There was a problem hiding this comment.
Similar to the previous comment, ensure that the href attribute set to # is intentional. Consider providing a valid URL or handling the click event with JavaScript.
src/index.html
Outdated
| <a | ||
| class="nav__link returned" | ||
| href="#" |
There was a problem hiding this comment.
Ensure that the href attribute set to # is intentional. Consider providing a valid URL or handling the click event with JavaScript.
src/index.html
Outdated
| <a | ||
| class="nav__link returned" | ||
| data-qa="hover" | ||
| href="#" |
There was a problem hiding this comment.
The data-qa attribute is used for testing purposes. Ensure that it is necessary and does not interfere with production code.
src/index.html
Outdated
| <a | ||
| class="nav__link returned" | ||
| href="#" |
There was a problem hiding this comment.
Ensure that the href attribute set to # is intentional. Consider providing a valid URL or handling the click event with JavaScript.
| <a | ||
| class="nav__link returned" | ||
| href="#" |
There was a problem hiding this comment.
Ensure that the href attribute set to # is intentional. Consider providing a valid URL or handling the click event with JavaScript.
| flex-direction: row; | ||
| justify-content: flex-end; | ||
| width: 100%; | ||
| min-width: 1024px; |
There was a problem hiding this comment.
The use of min-width: 1024px; on the .header class might cause layout issues on smaller screens. Consider using media queries to ensure responsiveness across different devices.
src/style.css
Outdated
| position: absolute; | ||
| width: 40px; | ||
| height: 40px; | ||
| left: 50px; |
There was a problem hiding this comment.
The .logo class uses position: absolute;, which might lead to overlapping issues with other elements. Ensure that this positioning is necessary and does not affect the layout negatively.
src/style.css
Outdated
| display: flex; | ||
| flex-direction: row; | ||
| list-style-type: none; | ||
| align-content: flex-end; |
There was a problem hiding this comment.
The align-content: flex-end; property on .nav__list has no effect because the flex container is set to flex-direction: row;. Consider removing it or adjusting the flex properties if needed.
There was a problem hiding this comment.
Great job on addressing many of the previous issues! 🎉 While there are still a few areas to improve, such as removing the serif fallback from the font configuration and ensuring efficient CSS reset, your progress is commendable. Remember to focus on using semantic tags effectively and applying the correct height settings to the nav links. These adjustments will enhance your code's functionality and alignment with the task requirements. Keep up the good work, and use this opportunity to refine your skills further. Your dedication to learning is evident, and I'm confident you'll continue to improve! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <header class="header"> | ||
| <a | ||
| class="logo" | ||
| href="index.html" | ||
| > | ||
| <img | ||
| src="/src/images/logo.png" | ||
| alt="logo" | ||
| /> | ||
| </a> | ||
| <nav class="nav"> | ||
| <ul class="nav__list"> | ||
| <li> | ||
| <a | ||
| class="nav__link is-active" | ||
| href="#" | ||
| > | ||
| apple | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| samsung | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| smartphones | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| class="nav__link" | ||
| data-qa="hover" | ||
| href="#" | ||
| > | ||
| laptops & computers | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| gadgets | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| tablets | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| photo | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| class="nav__link returned" | ||
| href="#" | ||
| > | ||
| video | ||
| </a> | ||
| </li> | ||
| </ul> | ||
| </nav> | ||
| </header> |
There was a problem hiding this comment.
The header height should be set for the nav links, not the header itself. Ensure that the height is applied to the appropriate elements as specified in the task requirements.
| <nav class="nav"> | ||
| <ul class="nav__list"> | ||
| <li> | ||
| <a | ||
| class="nav__link is-active" | ||
| href="#" | ||
| > | ||
| apple | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| samsung | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| smartphones | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| class="nav__link" | ||
| data-qa="hover" | ||
| href="#" | ||
| > | ||
| laptops & computers | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| gadgets | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| tablets | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| class="nav__link" | ||
| href="#" | ||
| > | ||
| photo | ||
| </a> | ||
| </li> | ||
| <li> | ||
| <a | ||
| class="nav__link returned" | ||
| href="#" | ||
| > | ||
| video | ||
| </a> | ||
| </li> | ||
| </ul> | ||
| </nav> |
There was a problem hiding this comment.
Consider using more semantic tags for the navigation section. For example, using <nav> and <ul> is correct, but ensure that all semantic tags required by the task are used appropriately throughout the HTML structure.
| body { | ||
| margin: 0; | ||
| html { | ||
| font-family: Roboto, serif; |
There was a problem hiding this comment.
The task requires using only the Roboto font with specific settings. The inclusion of serif as a fallback does not meet the task requirements. Please adjust this to use only the Roboto font.
| .nav__list :not(:first-child) { | ||
| margin-left: 20px; |
There was a problem hiding this comment.
The task specifies not to use the flex gap property for indents. Instead, use margin to achieve the desired spacing. Ensure that the spacing between elements is achieved using margin as per the task requirements.
