-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
New change in file index and style #6269
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Review Summary
Hey there! 🎉 I'm approving this pull request so you can move forward. There are a few things to keep in mind for future improvements.
Here's a breakdown:
- HTML: The logo should be wrapped in an
<a>
tag. This will make it a clickable link, which is what the task asks for. The logo should also be a link with an image inside (from[src/images](src/images)
), but it should not be a part of the<nav>
. - CSS:
- Font Family: Make sure to add a fallback font to your
font-family
property in thebody
selector. This ensures that if the primary font doesn't load, the page still looks readable. Something likefont-family: 'Your Font', sans-serif;
would work. - Header Width: Remove the
max-width
from the header. The header should stretch across the entire page. - CSS Variables: Using CSS variables for the blue color will make it easier to maintain your styles. Define it once at the top (e.g.,
:root { --blue-color: #yourblue; }
) and reuse it throughout your CSS. - Uppercase Text: Use CSS (
text-transform: uppercase;
) to transform the nav links to uppercase instead of typing them that way in the HTML. - Active Link: Implement the blue line below the active link using a pseudo-element (
::after
or::before
). This is a cleaner and more efficient way to achieve the effect. - Google Fonts Configuration: Google Fonts Configuration does not follow requirements. You should select ONLY roman style, medium (500) weight and normal width for embedding.
- Header Height: The header height is set in
header
styles, but it should be set for the links.
- Font Family: Make sure to add a fallback font to your
Remember to take another look at the comments to fully understand the suggested changes and why they are important. Keep up the great work, and don't hesitate to ask questions as you continue learning! 😊
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
<img | ||
src="images/logo.png" | ||
alt="logo" | ||
class="logo" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logo should be wrapped in a link (<a>
tag). According to the task description, "the logo should also be a link with an image inside (from src/images). But it should not be a part of the <nav>
".
@@ -1,3 +1,64 @@ | |||
body { | |||
margin: 0; | |||
font-family: 'Roboto', sans-serif; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a fallback font to the font-family
property. According to the checklist, you should "Remember to use fallback fonts - alternative font-family in case the main one doesn't work".
} | ||
.menu li:nth-child(1) a, | ||
.menu li:nth-child(4) a { | ||
color: blue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's recommended to use CSS variables for the blue color. The checklist says: "CSS Variable is used for a blue color".
|
||
.menu a.is-active::after { | ||
color: blue; | ||
text-decoration: none; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pseudo-element should be used for a blue line below the active link. The checklist says: "Pseudo-element is used for a blue line below the active link".
} | ||
|
||
header { | ||
max-width: 1200px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The header's max-width
should be removed. The task description says that "the header should stretch the full page width (don't use a horizontal margin)".
Demo
https://borismund.github.io/layout_moyo-header/
Test report link
https://mate-academy.github.io/layout_moyo-header/report/html_report/