Develop #6064
Develop #6064
Conversation
MariMajorana
commented
Jan 30, 2025
MariMajorana
commented
- 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 ❗️
There was a problem hiding this comment.
Thank you for your hard work on this project! 😊 There are a few critical issues that need to be addressed before we can approve the pull request. Specifically, the absence of href attributes in the <a> tags for navigation links and the logo significantly impacts the functionality and accessibility of your code. Additionally, the font embedding does not meet the task's specific requirements. Once these issues are resolved, your solution will be much stronger. Keep up the great work, and I'm confident you'll nail it with the next iteration! 🚀
✨ 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
| @@ -17,6 +29,45 @@ | |||
| /> | |||
| </head> | |||
| <body> | |||
| <h1>Moyo header</h1> | |||
| <header class="header"> | |||
| <a class="header__logo logo"> | |||
There was a problem hiding this comment.
The <a> tag is used for the logo but lacks an href attribute. This can be misleading for accessibility tools and users, as it suggests a link that is not functional. Consider adding an href attribute or using a different tag if it's not meant to be a link.
src/index.html
Outdated
| <nav class="header__nav nav"> | ||
| <ul class="nav__links"> | ||
| <li class="nav__item"> | ||
| <a class="nav__link is-active">apple</a> |
There was a problem hiding this comment.
The <a> tag for navigation links should have an href attribute to specify the destination URL. This is important for accessibility and functionality.
src/index.html
Outdated
| <a class="nav__link is-active">apple</a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a class="nav__link">samsung</a> |
There was a problem hiding this comment.
The <a> tag for navigation links should have an href attribute to specify the destination URL. This is important for accessibility and functionality.
src/index.html
Outdated
| <a class="nav__link">samsung</a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a class="nav__link">smartphones</a> |
There was a problem hiding this comment.
The <a> tag for navigation links should have an href attribute to specify the destination URL. This is important for accessibility and functionality.
src/index.html
Outdated
| <a class="nav__link">photo</a> | ||
| </li> | ||
| <li class="nav__item"> | ||
| <a class="nav__link">video</a> |
There was a problem hiding this comment.
The <a> tag for navigation links should have an href attribute to specify the destination URL. This is important for accessibility and functionality.
src/index.html
Outdated
| <a class="header__logo logo"> | ||
| <img | ||
| class="logo__image" | ||
| alt="logo" |
There was a problem hiding this comment.
The alt attribute for the logo image is quite generic ('logo'). Consider providing a more descriptive text to improve accessibility.
src/style.css
Outdated
| @@ -1,3 +1,68 @@ | |||
| body { | |||
| margin: 0; | |||
| padding: 0; | |||
| font-family: Roboto, serif; | |||
There was a problem hiding this comment.
Consider using a fallback font stack for better cross-browser compatibility. For example, font-family: 'Roboto', 'Helvetica Neue', Arial, sans-serif;.
src/style.css
Outdated
|
|
||
| .is-active, | ||
| .nav__link:hover { | ||
| color: #00acdc; |
There was a problem hiding this comment.
The color #00acdc is hardcoded. Consider using CSS variables for colors to improve maintainability and consistency across the stylesheet.
src/style.css
Outdated
| bottom: 0; | ||
| height: 4px; | ||
| border-radius: 8px; | ||
| background-color: #00acdc; |
There was a problem hiding this comment.
The background color #00acdc is hardcoded. Consider using CSS variables for colors to improve maintainability and consistency across the stylesheet.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
There was a problem hiding this comment.
well done =)
I saw that all your https://github.com/mate-academy/layout_moyo-header/actions/runs/13080768616/job/36503619058?pr=6064 have passed, but the pipeline has failed, did you forgot to push some changes?
