add solution #5350

oandriitsiv · 2024-09-11T09:29:22Z

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
[x Pseudo-element is used for a blue line below the active link
Code follows all the [Code Style Rules ❗️]

mateacademy-ai-mentor

Hey there! Good effort on the code, but we've got a few important issues to address before we can merge your pull request. 🛠️ There are some typos and best practices that need to be corrected, like the class names and the use of tag names for styling. These changes are crucial for maintaining code consistency and readability. Also, don't forget to add a fallback font-family and to keep an eye on the line length for better readability. Keep up the good work, and with these adjustments, your code will be in great shape! 👍

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

mateacademy-ai-mentor · 2024-09-11T09:29:51Z

src/index.html

+    <header class="container">
+      <a
+        href="index.html"
+        class="logo"
+      >
+        <img
+          src="./images/logo.png"
+          alt="Logo"
+        />


Add empty lines between multiline sibling blocks of HTML to add some 'air' and simplify reading. For example, add an empty line before the
element on line 30.

Add empty line before the element on line 30.

src/index.html

mateacademy-ai-mentor · 2024-09-11T09:29:51Z

src/index.html

+          <li class="nav__item">
+            <a
+              href="#"
+              class="is-activ nav__link"


Don't use spaces in <a> tag's href property. Also, ensure that class names represent the meaning of the content. The class name 'is-activ' seems to be a typo and should be 'is-active' if it's meant to indicate the active state.

Delete spaces in tag's href property. Fixed the error in the class name 'is-active'

mateacademy-ai-mentor · 2024-09-11T09:29:51Z

src/index.html

+              class="nav__link"
+              data-qa="hover"
+            >
+              LAPTOPS & COMPUTERS


Lines of code should have 80 characters max for better readability. The text inside the tag on line 62 is too long and should be broken into multiple lines if necessary.

Text inside the tag on line 62 cannot be broken.

mateacademy-ai-mentor · 2024-09-11T09:29:52Z

src/style.css

@@ -1,3 +1,70 @@
 body {
+  font-family: Roboto, sans-serif;


Consider adding a fallback font-family in case 'Roboto' fails to load for some reason. For example, 'sans-serif' can be a good fallback.

mateacademy-ai-mentor · 2024-09-11T09:29:52Z

src/style.css

  margin: 0;
 }
+
+.logo.png {


Avoid using tag names for styling. Use class selectors instead. The .logo.png looks like a filename, not a class name.

changed name class .logo

mateacademy-ai-mentor · 2024-09-11T09:29:52Z

src/style.css

+  margin-left: 50px;
+}
+
+.nav_item {


It seems there is a typo in the class name .nav_item. According to the naming convention used in the rest of the file, it should be .nav__item.

Corrected the typo in the class .nav__item

mateacademy-ai-mentor · 2024-09-11T09:29:52Z

src/style.css

+  position: relative;
+}
+
+.nav__link.is-activ {


The class .nav__link.is-activ seems to have a typo. If it's meant to indicate the active state, it should be .nav__link.is-active.

Corrected the typo in the class .nav__link.is-active

mateacademy-ai-mentor · 2024-09-11T09:29:52Z

src/style.css

+  height: 4px;
+  content: '';
+  background-color: #00acdc;
+  border-color: #00acdc;


The border-color property in .nav__link.is-activ::after is redundant since this pseudo-element does not have a border. You can remove this property.

delete property the border-color.

mateacademy-ai-mentor

Great work on your Pull Request! 🌟 I've decided to approve it because there are no critical issues that impact the functionality or security of your code. However, please take note of the suggestions made to enhance your code's readability, maintainability, and responsiveness. Keep up the good coding practices, and remember to always consider fallback options and accessibility. Happy coding! 💻

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

mateacademy-ai-mentor · 2024-09-18T10:44:49Z

src/index.html

-    <h1>Moyo header</h1>
+    <header class="container">
+      <a
+        href="index.html"


Don't use spaces in <a> tag's href property. Anchor links should start with the # symbol if they are meant to be page anchors. If this is meant to be a link to the homepage, consider using / for clarity.

mateacademy-ai-mentor · 2024-09-18T10:44:49Z

src/index.html

+      >
+        <img
+          src="./images/logo.png"
+          alt="logo-moyo"


The alt attribute should describe the image content. Instead of 'logo-moyo', a more descriptive alternative text like 'Moyo company logo' would be more informative.

mateacademy-ai-mentor · 2024-09-18T10:44:49Z

src/index.html

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


Add empty lines between multiline sibling blocks of HTML for better readability. In this case, add an empty line before the <nav> tag to separate it from the <header> tag.

mateacademy-ai-mentor · 2024-09-18T10:44:49Z

src/style.css

@@ -1,3 +1,69 @@
 body {
+  font-family: Roboto, sans-serif;


Consider adding a fallback font-family for Roboto to ensure text is displayed with a default sans-serif font if Roboto fails to load.

mateacademy-ai-mentor · 2024-09-18T10:44:49Z

src/style.css

+
+.container {
+  display: flex;
+  height: 60px;


Avoid fixing the container size unless it's a specific requirement. It's better to let the content determine the height to prevent overflow or accidental scroll bars.

mateacademy-ai-mentor · 2024-09-18T10:44:49Z

src/style.css

+
+.nav__list {
+  display: flex;
+  width: 1200px;


It's not recommended to set a fixed width for elements such as '.nav__list' unless it's a specific requirement. Consider using max-width or relative units like percentages to maintain responsiveness.

mateacademy-ai-mentor · 2024-09-18T10:44:50Z

src/style.css

+}
+
+.nav__item {
+  display: block;


The '.nav__item' class has a 'display: block;' property which is unnecessary since
elements are block-level by default.

mateacademy-ai-mentor · 2024-09-18T10:44:50Z

src/style.css

+}
+
+.nav__link.is-active {
+  display: inline-block;


The '.nav__link.is-active' class has a 'display: inline-block;' property which might not be necessary. If you're trying to override a previous 'display' value, ensure it's required; otherwise, it can be removed.

volodymyr-soltys97

Hi, you need to fix all tests, check my comments, if you have any questions feel free in the fe_chat

volodymyr-soltys97 · 2024-09-18T11:08:11Z

src/index.html

@@ -17,6 +17,78 @@
    />
  </head>
  <body>
-    <h1>Moyo header</h1>
+    <header class="container">


Suggested change

<header class="container">

<header class="header">

volodymyr-soltys97 · 2024-09-18T11:08:30Z

src/index.html

+          </li>
+          <li class="nav__item">


Add the blank line between the links

Suggested change

</li>

<li class="nav__item">

</li>

<li class="nav__item">

volodymyr-soltys97 · 2024-09-18T11:09:04Z

src/style.css

+  font-family: Roboto, sans-serif;
+  font-size: 12px;


Move font styles to the html selector

volodymyr-soltys97 · 2024-09-18T11:09:17Z

src/style.css

+  font-family: Roboto, sans-serif;
+  font-size: 12px;
+
+  --hover-color: #00acdc;


Create a variable with :root selector

volodymyr-soltys97 · 2024-09-18T11:09:59Z

src/style.css

+.container {
+  display: flex;
+  height: 60px;
+  align-items: center;
+  align-content: space-around;
  margin: 0;
 }


Suggested change

.container {

display: flex;

height: 60px;

align-items: center;

align-content: space-around;

margin: 0;

}

.header {

display: flex;

align-items: center;

justify-content: space-between;

padding: 0 50px;

box-sizing: border-box;

}

volodymyr-soltys97 · 2024-09-18T11:10:19Z

src/style.css

+.nav__list {
+  display: flex;
+  width: 1200px;
+  justify-content: flex-end;
+  list-style: none;
+  padding: 0;
+  margin-left: 50px;
+}


Suggested change

.nav__list {

display: flex;

width: 1200px;

justify-content: flex-end;

list-style: none;

padding: 0;

margin-left: 50px;

}

.nav__list {

display: flex;

list-style: none;

padding: 0;

}

volodymyr-soltys97 · 2024-09-18T11:11:22Z

src/style.css

+}
+


Suggested change

}

}

body {

margin: 0;

}

volodymyr-soltys97 · 2024-09-18T11:12:14Z

src/style.css

+.nav__link {
+  color: #000;
+  text-decoration: none;
+  text-transform: uppercase;
+  font-size: 12px;
+  line-height: 14px;
+  padding: 5px;
+  position: relative;
+}


Suggested change

.nav__link {

color: #000;

text-decoration: none;

text-transform: uppercase;

font-size: 12px;

line-height: 14px;

padding: 5px;

position: relative;

}

.nav__link {

color: #000;

text-decoration: none;

text-transform: uppercase;

font-size: 12px;

line-height: 60px;

display: inline-block:

height: 60px;

}

volodymyr-soltys97 · 2024-09-18T11:12:32Z

src/style.css

+}
+
+.nav__link:hover {
+  color: #00acdc;


Create a variable for repeated color

anastasiiavorobiova

Try to pass all tests

Links should have the height of the header

Consider removing default margins here

Check the size of the logo

Links should be centered vertically
Also, check font styles

anastasiiavorobiova · 2024-09-18T14:03:02Z

src/index.html

+      <a href="index.html"
+        class="logo" >


Suggested change

<a href="index.html"

class="logo" >

<a

href="index.html"

class="logo"

>

add solution

70bf8ce

mateacademy-ai-mentor suggested changes Sep 11, 2024

View reviewed changes

oandriitsiv added 4 commits September 11, 2024 13:45

Merge branch 'master' into develop

ac9cf94

add correction

e730021

fixed errors

0918c54

solution 1

f2b6bd2

oandriitsiv requested a review from mateacademy-ai-mentor September 18, 2024 10:44

mateacademy-ai-mentor approved these changes Sep 18, 2024

View reviewed changes

volodymyr-soltys97 suggested changes Sep 18, 2024

View reviewed changes

oandriitsiv added 6 commits September 18, 2024 13:28

rename class header

4663185

add the blank line between the link

237fa01

move font styles to the html selector

f6d5334

change style header

fe244af

change style link

de6f34c

fixed style

60d957d

oandriitsiv requested a review from mateacademy-ai-mentor September 18, 2024 13:56

anastasiiavorobiova suggested changes Sep 18, 2024

View reviewed changes

add solution #5350

Are you sure you want to change the base?

add solution #5350

Conversation

oandriitsiv commented Sep 11, 2024

mateacademy-ai-mentor 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateacademy-ai-mentor 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

volodymyr-soltys97 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

anastasiiavorobiova left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment