Skip to content
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.

Sign up for GitHub

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

add task solution #5368

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

add task solution #5368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6a8d5e0
add task solution
bodik-s4 Sep 16, 2024
79d67a7
add href attributes in each <a> tag, change the alt attribute and add…
bodik-s4 Sep 16, 2024
7b9a85b
fix the mistakes
bodik-s4 Sep 16, 2024
4ec468d
add solution with margin
bodik-s4 Sep 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,8 @@ The page should match the design Pixel Perfect: all the sizes, colors and distan

❗️ Replace `<your_account>` with your Github username and copy the links to `Pull Request` description:

- [DEMO LINK](https://<your_account>.github.io/layout_moyo-header/)
- [TEST REPORT LINK](https://<your_account>.github.io/layout_moyo-header/report/html_report/)
- [DEMO LINK](https://bodik-s4.github.io/layout_moyo-header/)
- [TEST REPORT LINK](https://bodik-s4.github.io/layout_moyo-header/report/html_report/)

❗️ Copy this `Checklist` to the `Pull Request` description after links, and put `- [x]` before each point after you checked it.

Expand Down
101 changes: 100 additions & 1 deletion src/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,111 @@
content="ie=edge"
/>
<title>Moyo header</title>
<link
rel="preconnect"
href="https://fonts.googleapis.com"
/>
<link
rel="preconnect"
href="https://fonts.gstatic.com"
/>
<link
href="https://fonts.googleapis.com/css2?family=Roboto:ital,wght@0,100;0,300;0,400;0,500;0,700;0,900;1,100;1,300;1,400;1,500;1,700;1,900&display=swap"
rel="stylesheet"
/>
<link
rel="stylesheet"
href="./style.css"
/>
</head>
<body>
<h1>Moyo header</h1>
<header class="header">
<a
class="header__link"
href="images/logo.png"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't use spaces in <a> tag's href property. It seems like you're linking to an image, which should be displayed using an <img> tag, not linked via <a> with href.

>
Comment on lines +35 to +36
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The href attribute in the anchor tag should not point to an image file directly unless it's meant for download. If it's meant to be a link to the homepage, it should have a path like / or #home.

<img
src="images/logo.png"
alt="moyo-logo"
/>
Comment on lines +33 to +40
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The href attribute of the anchor tag pointing to the logo image should likely be the root path or a specific page instead of the image file itself, unless intended for download or a similar function.

</a>

<nav class="nav">
<ul class="nav__list">
<li class="nav__item">
<a
class="nav__link is-active"
href="#apple"
>
Apple
</a>
</li>

<li class="nav__item">
<a
class="nav__link"
href="#samsung"
>
Samsung
</a>
</li>

<li class="nav__item">
<a
class="nav__link"
href="#smartphones"
>
Smartphones
</a>
</li>

<li class="nav__item">
<a
class="nav__link"
href="#laptops-computers"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The href attribute in the anchor tag should not contain spaces. Replace href="#laptops-computers" with href="#laptops-computers" to correct the URL.

data-qa="hover"
Comment on lines +75 to +76
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using spaces in the href attribute of the <a> tag, as anchor links start with the # symbol. The correct value should be #laptops-computers.

>
Laptops & Computers
</a>
</li>

<li class="nav__item">
<a
class="nav__link"
href="#gadgets"
>
Gadgets
</a>
</li>

<li class="nav__item">
<a
class="nav__link"
href="#tablets"
>
Tablets
</a>
</li>

<li class="nav__item">
<a
class="nav__link"
href="#photo"
>
Photo
</a>
</li>

<li class="nav__item">
<a
class="nav__link"
href="#video"
>
Video
</a>
</li>
</ul>
</nav>
</header>
</body>
</html>
73 changes: 73 additions & 0 deletions src/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,76 @@
:root {
--active-color: #00acdc;
}

body {
margin: 0;
}

html {
font-family: Roboto, Arial, sans-serif;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider specifying fallback font-families in case 'Roboto' is not available. For instance, 'font-family: Roboto, Arial, sans-serif;' ensures that if 'Roboto' fails to load, 'Arial' will be used, and if both custom fonts fail, the browser's default sans-serif font will be used.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to use fallback fonts - alternative font-family in case the main one doesn't work. Consider adding a generic font family as a fallback after 'Arial', such as 'sans-serif'.

font-size: 12px;
font-weight: 500;
text-transform: uppercase;
}

.header {
position: relative;
display: flex;
align-items: center;
justify-content: space-between;
box-shadow: 0 2px 4px 0 #0000000d;
padding: 0 50px;
}

.nav__list {
display: flex;
list-style: none;
padding-left: 0;
margin: 0;
Comment on lines +25 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using tag names for styling. You should use classes instead to ensure that the styling is specific and doesn't inadvertently affect other list elements that might be added to the page in the future.

}

.nav__link {
position: relative;
display: flex;
height: 60px;
margin-left: 20px;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be consistent with vertical margins: it's recommended to apply either top or bottom margin, but not both, to elements to avoid unexpected margin collapsing. In this case, you have a margin-left, which is horizontal, but it's good practice to review all margins for consistency.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the checklist, be consistent with your vertical margins. It is recommended to use either top or bottom margin, but not both to avoid collapsing margins issues. In this case, 'margin-top' and 'margin-bottom' are set on '.header__link' (lines 60-61), which could potentially lead to such issues.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be consistent with your vertical margins (Add only top or only bottom margin, but not both). Here you have a left margin, which is fine, but make sure to avoid having both top and bottom margins for vertical consistency.

align-items: center;
text-decoration: none;
color: #000;
}

.nav__item {
cursor: default;
justify-content: center;
}

.is-active {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The '.is-active' class has a 'margin-left' property which is redundant since it's already defined in the '.nav__link' class (line 36) and '.is-active' is expected to override '.nav__link'. If the intention is to set 'margin-left' to 0 for the '.is-active' class, then this is correct. However, if the intention is to maintain the same 'margin-left' as '.nav__link', then this line can be removed.

color: var(--active-color);
}

.nav__link:hover {
color: var(--active-color);
}

.header__link {
display: flex;
width: 40px;
height: 40px;
margin-top: 10px;
margin-bottom: 10px;
Comment on lines +59 to +60
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be consistent with your vertical margins (Add only top or only bottom margin, but not both). Here you've added both top and bottom margins, consider using only one to maintain consistency and to avoid margin collapse issues.

}

.is-active::after {
content: '';
display: block;
height: 4px;
width: 100%;
background-color: var(--active-color);
position: absolute;
bottom: 0;
border-radius: 8px;
}

.nav__item:first-child .nav__link {
margin-left: 0;
}
Loading