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 1 commit
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
65 changes: 64 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,75 @@
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="logo"
Copy link

Choose a reason for hiding this comment

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

alt attribute should describe the image content. Instead of just 'logo', describe the logo in a way that would make sense to users who cannot see the image.

/>
</a>

<nav class="nav">
<ul class="nav__list">
<li class="nav__item">
<a class="nav__link is-active">Apple</a>
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 is missing in the <a> tag. Remember to provide a valid reference to make the link functional. Also, avoid using spaces in the href value.

</li>

<li class="nav__item">
<a class="nav__link">Samsung</a>
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 is missing in the <a> tag. Remember to provide a valid reference to make the link functional.

</li>

<li class="nav__item">
<a class="nav__link">Smartphones</a>
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 is missing in the <a> tag. Remember to provide a valid reference to make the link functional.

</li>

<li class="nav__item">
<a
class="nav__link"
data-qa="hover"
>
Laptops & Computers
</a>
</li>

<li class="nav__item">
<a class="nav__link">Gadgets</a>
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 is missing in the <a> tag. Remember to provide a valid reference to make the link functional.

</li>

<li class="nav__item">
<a class="nav__link">Tablets</a>
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 is missing in the <a> tag. Remember to provide a valid reference to make the link functional.

</li>

<li class="nav__item">
<a class="nav__link">Photo</a>
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 is missing in the <a> tag. Remember to provide a valid reference to make the link functional.

</li>

<li class="nav__item">
<a class="nav__link">Video</a>
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 is missing in the <a> tag. Remember to provide a valid reference to make the link functional.

</li>
</ul>
</nav>
</header>
</body>
</html>
68 changes: 68 additions & 0 deletions src/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,71 @@
:root {
--active-color: #00acdc;
}

body {
margin: 0;
}

html {
font-family: Roboto, 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 adding fallback font-families to improve robustness of your CSS in case the primary font isn't available. For example, 'Roboto, Arial, 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;
}

.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.

margin-left: 0;
Copy link

Choose a reason for hiding this comment

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

Suggested change
margin-left: 0;

it's bad practice to reset the margin in this class, because we can make the third link active, but we don't need in the third link margin-left:0

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;
}
Loading