Develop #6118
Develop #6118
Conversation
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 There are a couple of critical issues that need to be addressed before we can approve the pull request. Firstly, the requirement to use a CSS variable for the blue color has not been met, as the color is directly used without a variable. Secondly, the Google Fonts configuration includes unnecessary italic styles, which do not align with the task requirements. Additionally, there's a potential issue with duplicate links in the navigation menu that should be clarified or corrected. Please make these changes to fully meet the task requirements. Keep up the great work, and I look forward to your updated submission! 🚀
✨ 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 class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/portal_brand/photo_video/" | ||
| class="nav__link" | ||
| > | ||
| Photo | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/portal_brand/photo_video/" | ||
| class="nav__link" | ||
| > | ||
| Video | ||
| </a> | ||
| </li> |
There was a problem hiding this comment.
The 'Photo' and 'Video' links both point to the same URL: 'https://www.moyo.ua/ua/portal_brand/photo_video/'. If these are intended to lead to different sections, ensure that the URLs are distinct. If they are meant to be the same, consider combining them into a single link or clarifying their purpose.
There was a problem hiding this comment.
Your Pull Request has been approved! 🎉 Great job on addressing the previous issues effectively. Here’s a summary of the review:
-
CSS Variable for Blue Color: You've successfully implemented a CSS variable
--accent-colorfor the blue color, which is used appropriately in the.nav__link:hoverand.is-activeclasses. This resolves the previous concern regarding the use of a CSS variable. -
Google Fonts Configuration: The configuration now correctly includes only the Roboto font with the medium (500) weight and normal width, aligning with the task requirements. Well done on removing unnecessary italic styles.
-
Navigation Menu Links: Although there was a potential issue with duplicate links, you've ensured that each link has a unique
hrefattribute, resolving the concern.
Your solution meets the specified task requirements and constraints. However, please review the comments regarding the potential issue with duplicate links to ensure clarity and functionality. Addressing such issues will help you avoid similar problems in the future. Keep up the great work and continue learning from each task! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <link | ||
| rel="preconnect" | ||
| href="https://fonts.googleapis.com" | ||
| /> | ||
| <link | ||
| rel="preconnect" | ||
| href="https://fonts.gstatic.com" | ||
| crossorigin="anonymous" | ||
| /> | ||
| <link | ||
| href="https://fonts.googleapis.com/css2?family=Roboto:wght@500&display=swap" | ||
| rel="stylesheet" | ||
| /> |
There was a problem hiding this comment.
The Google Fonts configuration now correctly includes only the required font weights without unnecessary italic styles. This aligns with the task requirements.
| <a | ||
| href="https://www.moyo.ua" | ||
| class="logo" | ||
| > | ||
| <img | ||
| src="images/logo.png" | ||
| alt="Moyo" | ||
| /> | ||
| </a> | ||
| <nav class="nav"> | ||
| <ul class="nav__list"> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/portal_brand/apple_1/" | ||
| class="nav__link is-active" | ||
| > | ||
| Apple | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/brands/samsung/" | ||
| class="nav__link" | ||
| > | ||
| Samsung | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/portal_brand/smartphones_phones/" | ||
| class="nav__link" | ||
| > | ||
| Smartphones | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/portal_brand/comp_and_periphery/" | ||
| class="nav__link" | ||
| data-qa="hover" | ||
| > | ||
| Laptops & Computers | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/portal_brand/comp_components/" | ||
| class="nav__link" | ||
| > | ||
| Gadgets | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/portal_brand/beauty_health/" | ||
| class="nav__link" | ||
| > | ||
| Tablets | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/portal_brand/photo_video/" | ||
| class="nav__link" | ||
| > | ||
| Photo | ||
| </a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a | ||
| href="https://www.moyo.ua/ua/foto_video/video_tech/protected_cam/" | ||
| class="nav__link" | ||
| > | ||
| Video | ||
| </a> |
There was a problem hiding this comment.
There is a potential issue with duplicate links in the navigation menu. Ensure that each link is unique and serves a distinct purpose to avoid confusion for users.
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 ❗️
The Google Fonts Configuration follows requirements.