moyo header task solution #3407

arturgorniak · 2023-06-28T13:31:16Z

https://arturgorniak.github.io/layout_moyo-header/
https://arturgorniak.github.io/layout_moyo-header/report/html_report/

Radoslaw-Czerniawski

Apart from the comments left, your header should have a drop-shadow (I copied it from figma)
box-shadow: 0px 2px 4px 0px #0000000D;

Radoslaw-Czerniawski · 2023-06-28T15:24:59Z

readme.md

@@ -1,7 +1,7 @@
 # Moyo header
 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://arturgorniak.github.io/layout_moyo-header/)


You removed ( in these links, should be:

[DEMO LINK](https://arturgorniak.github.io/layout_moyo-header/)

thx to that you demo link won't look like a full url but rather like this -> DEMO LINK

Radoslaw-Czerniawski · 2023-06-28T15:25:24Z

src/index.html

@@ -9,6 +9,50 @@
    <link rel="stylesheet" href="./style.css">
  </head>
  <body>
-    <h1>Moyo header</h1>
+    <header class="header">
+      <a href="index.html" class="logo_link">


it should lead to #

Radoslaw-Czerniawski · 2023-06-28T15:31:36Z

src/index.html

-    <h1>Moyo header</h1>
+    <header class="header">
+      <a href="index.html" class="logo_link">
+        <img src="./images/logo.png"


Again take a look at the checklist, when you have 2 attributes your tag should look like this:

<img src="./images/logo.png" alt="Moyo Header Logo">

however in this case as these attributes have some long values I would format it like so:

<img src="./images/logo.png" alt="Moyo Header Logo" >

Take notice that I have change you alt attribute's value, be more precise when describing what should be displayed in the img. This alt attribute is the only thing that's visible for screen readers / robots reading your sites contents / people when your image doesn't load.

Radoslaw-Czerniawski · 2023-06-28T15:34:18Z

src/index.html

+          </li>
+
+          <li class="nav_item" data-qa="hover">
+            <a href="laptops"


You have forgotten about the # in href, this redirects to non-existing endpoint:

Please test your implementation before pr's

Also indent here needs to be corrected:

<a href="#laptops" class="nav_link"> Laptops & Computers </a>

Radoslaw-Czerniawski · 2023-06-28T15:36:06Z

src/index.html

+        >
+      </a>
+
+      <nav class="nav">


When you have repeated tags there is no need for empty lines between them, for example:

<ul> <li></li> <li></li> <li></li> <li></li> </ul>

Radoslaw-Czerniawski · 2023-06-28T15:47:32Z

src/style.css

+  display: flex;
+}
+
+.logo_link {


This should be enough here, rest is redundant:

.logo_link { display: flex; }

Bump this Radek mention, you can refactor these styles as suggested

Radoslaw-Czerniawski · 2023-06-28T15:57:26Z

src/style.css

+  padding: 0;
+}
+
+.nav_list > :last-child {


:last-child should be used on the child class rather than parent so:

~~.nav_list > :last-child { margin-right: 0; }~~

.nav_item { margin-right: 20px; } .nav_item:last-child { margin-right: 0; }

Radoslaw-Czerniawski · 2023-06-28T15:59:54Z

src/style.css

+  align-items: center;
+  font-size: 12px;
+  line-height: 15px;
+  padding: 23px 0;


Use rounded values divisible by 5 as it makes any calculations and positioning much easier.

Bump this also, please set this padding value to 22px or 24px

Radoslaw-Czerniawski · 2023-06-28T16:07:05Z

src/style.css

+  width: 100%;
+  height: 4px;
+  border-radius: 8px;
+  background-color: #00acdc;


Generally it's a good practice to keep all your colors as variables at the beggining of the file in the :root pseudoclass and later use these variables:

:root { --light-blue: #00acdc; } ... background-color: var(--light-blue); ...

This is better for scalability of your app, it will be much easier to change your color scheme (or theme to dark for example) if all your colors are variables stored in one place. Also take notice of the naming of the variable I used here, don't use names likemain-color but rather names that describe it like here light-blue

darokrk

UI looks good 🏅 few fixes needed and we got it 😃

darokrk · 2023-06-30T06:17:16Z

src/index.html

+          <li class="nav_item">
+            <a href="#smartphones" class="nav_link">Smartphones</a>
+          </li>
+          <li class="nav_item" data-qa="hover">


The 'data-qa' attribute has been added to the 'li' element, whereas it should be added to the 4th 'a' link as per the task description.

darokrk · 2023-06-30T06:21:58Z

src/style.css

+  display: flex;
+}
+
+.logo_link {


Bump this Radek mention, you can refactor these styles as suggested

darokrk · 2023-06-30T06:23:05Z

src/style.css

+  align-items: center;
+  font-size: 12px;
+  line-height: 15px;
+  padding: 23px 0;


Bump this also, please set this padding value to 22px or 24px

darokrk · 2023-06-30T06:25:24Z

src/index.html

@@ -9,6 +9,41 @@
    <link rel="stylesheet" href="./style.css">
  </head>
  <body>
-    <h1>Moyo header</h1>
+    <header class="header">
+      <a href="#home" class="logo_link">


Following the BEM naming convention please add a double underscore instead of one for block elements styles class="logo__link"

Please change that in the whole project HTML and CSS :)

arturgorniak · 2023-06-30T08:48:01Z

I made changes as sugested, can someone check this task too? I have waited quite a bit of time already and noone rated it.
mate-academy/layout_html-form#800

DorotaLeniecDev

Add propper shadow for header, and deploy your page using npm run deploy after the fixes so they are visible

DorotaLeniecDev · 2023-06-30T09:49:49Z

src/index.html

+            <a
+              href="#laptops"
+              class="nav__link"
+              data-qa="hover">Laptops & Computers
+            </a>


Should be:

Suggested change

<a

href="#laptops"

class="nav__link"

data-qa="hover">Laptops & Computers

</a>

<a

href="#laptops"

class="nav__link"

data-qa="hover"

>

Laptops & Computers

</a>

arturgorniak · 2023-06-30T12:36:18Z

fixed already, sorry i forgot about the shadow previously.

Radoslaw-Czerniawski

Accepted, layouts looks very good, do mind the spacing in future prs

Radoslaw-Czerniawski · 2023-06-30T17:43:18Z

src/index.html

+          src="./images/logo.png"
+          alt="Moyo Header Logo"
+        >
+      </a>


Here should be an empty space, this a tag is a sibling to nav, generally the rule is (apart from the exception with repeated tags) that the siblings should have an empty space between them:

<div> <span></span> <p></p> <div> <ul> <li></li> <li></li> </ul> </div> </div>

arturgorniak added 4 commits June 28, 2023 14:49

add test solution

6b821cc

add test solution

5dadb73

final changes

e43245b

readme

e71aa9f

Radoslaw-Czerniawski suggested changes Jun 28, 2023

View reviewed changes

implementing requested changes

14f50a8

arturgorniak requested a review from Radoslaw-Czerniawski June 29, 2023 19:40

darokrk suggested changes Jun 30, 2023

View reviewed changes

final changes

a3a21b8

arturgorniak requested a review from darokrk June 30, 2023 08:45

DorotaLeniecDev suggested changes Jun 30, 2023

View reviewed changes

arturgorniak added 2 commits June 30, 2023 14:34

add box shadow

52d8e65

add box shadow

661b3df

arturgorniak requested a review from DorotaLeniecDev June 30, 2023 12:36

Radoslaw-Czerniawski approved these changes Jun 30, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

moyo header task solution #3407

moyo header task solution #3407

arturgorniak commented Jun 28, 2023 •

edited

Loading

Radoslaw-Czerniawski left a comment

Radoslaw-Czerniawski Jun 28, 2023

Radoslaw-Czerniawski Jun 28, 2023

Radoslaw-Czerniawski Jun 28, 2023

Radoslaw-Czerniawski Jun 28, 2023

Radoslaw-Czerniawski Jun 28, 2023

Radoslaw-Czerniawski Jun 28, 2023

Radoslaw-Czerniawski Jun 28, 2023

darokrk Jun 30, 2023

Radoslaw-Czerniawski Jun 28, 2023

Radoslaw-Czerniawski Jun 28, 2023

darokrk Jun 30, 2023

Radoslaw-Czerniawski Jun 28, 2023

darokrk left a comment

darokrk Jun 30, 2023

darokrk Jun 30, 2023

darokrk Jun 30, 2023

darokrk Jun 30, 2023

arturgorniak commented Jun 30, 2023

DorotaLeniecDev left a comment

DorotaLeniecDev Jun 30, 2023

arturgorniak commented Jun 30, 2023

Radoslaw-Czerniawski left a comment

Radoslaw-Czerniawski Jun 30, 2023

moyo header task solution #3407

Are you sure you want to change the base?

moyo header task solution #3407

Conversation

arturgorniak commented Jun 28, 2023 • edited Loading

Radoslaw-Czerniawski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darokrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturgorniak commented Jun 30, 2023

DorotaLeniecDev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturgorniak commented Jun 30, 2023

Radoslaw-Czerniawski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturgorniak commented Jun 28, 2023 •

edited

Loading