Develop #5361

artuom2283 · 2024-09-14T17:40:37Z

mateacademy-ai-mentor

Hey there! It looks like there are a few important issues that need to be addressed before we can approve your Pull Request. 🛠️ Remember to use valid URL formats in your href attributes, stick to lowercase, and avoid spaces and special characters. Also, make sure your indentation is consistent with 2 spaces, and watch out for those empty selectors and unnecessary spaces between elements. Adding a fallback font-family will also improve cross-browser compatibility. Keep up the good work, and once these changes are made, 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-14T17:41:00Z

src/index.html

-    <h1>Moyo header</h1>
+    <header class="header">
+      <div class="nav-logo">
+        <a href="homepage">


The href attribute in the anchor tag should not contain spaces and should start with a # if it's an anchor link. In this case, it seems like it should be a path, so ensure it's a valid path without spaces.

mateacademy-ai-mentor · 2024-09-14T17:41:00Z

src/index.html

+      <div class="nav-logo">
+        <a href="homepage">
+          <img
+            src="images\logo.png"


Use forward slashes (/) instead of backslashes (\) in the src attribute for consistency across different operating systems.

mateacademy-ai-mentor · 2024-09-14T17:41:00Z

src/index.html

+          <ul>
+            <li class="nav-item is-active apple-active">
+              <a
+                href="APPLE"


The href attribute value APPLE should be lowercase and should not contain spaces. It's better to use a meaningful path or an anchor link starting with #.

mateacademy-ai-mentor · 2024-09-14T17:41:00Z

src/index.html

+            </li>
+            <li class="nav-item is-active">
+              <a
+                href="LAPTOPS & COMPUTERS"


The href attribute value LAPTOPS & COMPUTERS contains spaces and special characters, which is not recommended. Use a valid URL format or an anchor link starting with #.

mateacademy-ai-mentor · 2024-09-14T17:41:00Z

src/index.html

+      <nav class="nav">
+        <div class="nav-links">
+          <ul>
+            <li class="nav-item is-active apple-active">


Remove unnecessary spaces between sibling elements. There should be an empty line between multiline sibling elements, but not spaces.

mateacademy-ai-mentor · 2024-09-14T17:41:00Z

src/style.css

+}
+
+html {
+  font-family: Roboto, sans-serif;


Consider adding a fallback font-family for better cross-browser compatibility. For example, 'Roboto', Arial, sans-serif.

mateacademy-ai-mentor · 2024-09-14T17:41:00Z

src/style.css

+  margin-top: 10px;
+  margin-bottom: 10px;
+  height: 60px;


Be consistent with your vertical margins. It's recommended to add only top or only bottom margin, but not both to prevent potential margin collapse issues. In this case, you might want to remove either 'margin-top' or 'margin-bottom'.

mateacademy-ai-mentor · 2024-09-14T17:41:00Z

src/style.css

+  justify-content: flex-end;
+}
+
+ul {


Avoid using tag names for styling. You should use classes instead of tag names like 'ul' to apply styles, except for the 'html' and 'body' tags.

src/style.css

vadiimvooo

Good job, but you have to fix your code according to comments below.

vadiimvooo · 2024-09-15T16:21:13Z

src/style.css

+/* li.is-active::after {
+  content: '';
+  position: absolute;
+  left: 0;
+  bottom: 0;
+  width: 100%;
+  height: 4px;
+  background-color: var(--link-color);
+  border-radius: 2px;
+  transform: scaleX(0);
+  transition: transform 0.15s ease-in-out;
+} */


Remove commented code

vadiimvooo · 2024-09-15T16:23:17Z

src/style.css

+  justify-content: flex-end;
+}
+
+ul.nav-list {


remove tags from selectors

vadiimvooo · 2024-09-15T16:24:12Z

src/index.html

+      <nav class="nav">
+        <div class="nav-links">
+          <ul class="nav-list">
+            <li class="nav-item is-active apple-active">


Suggested change

<li class="nav-item is-active apple-active">

<li class="nav-item">

vadiimvooo · 2024-09-15T16:24:26Z

src/index.html

+        </a>
+      </div>
+      <nav class="nav">
+        <div class="nav-links">


Suggested change

<div class="nav-links">

vadiimvooo · 2024-09-15T16:24:49Z

src/index.html

+            <li class="nav-item is-active apple-active">
+              <a
+                href="#apple"
+                class="is-active"


Suggested change

class="is-active"

class="nav__link is-active"

vadiimvooo · 2024-09-15T16:24:57Z

src/index.html

+                apple
+              </a>
+            </li>
+            <li class="nav-item is-active">


Suggested change

<li class="nav-item is-active">

<li class="nav-item">

is-active is class which should be used for Apple link.

The same for other elements.

volodymyr-soltys97

Good job 👍
Let's improve your code
Add a blue color for all links when hovering

Remove margins

volodymyr-soltys97 · 2024-09-15T19:11:47Z

src/index.html

    <link
      rel="stylesheet"
      href="./style.css"
    />
  </head>
  <body>
-    <h1>Moyo header</h1>
+    <header class="header">
+      <div class="nav-logo">


This div is redundant

Suggested change

<div class="nav-logo">

volodymyr-soltys97 · 2024-09-15T19:11:56Z

src/index.html

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


Suggested change

</div>

<nav class="nav">

</div>

<nav class="nav">

volodymyr-soltys97 · 2024-09-15T19:12:23Z

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-15T19:12:49Z

src/index.html

+            <a href="#samsung">samsung</a>
+          </li>
+          <li class="nav-item">
+            <a href="#smartphones">smartphones</a>


Add classes everywhere

Suggested change

<a href="#smartphones">smartphones</a>

<a class="nav__link" href="#smartphones">smartphones</a>

volodymyr-soltys97 · 2024-09-15T19:13:01Z

src/style.css

+  align-items: center;
+  justify-content: space-between;
+  padding: 0 50px;
+  margin: 10px 0;


Suggested change

margin: 10px 0;

volodymyr-soltys97 · 2024-09-15T19:13:20Z

src/style.css

+  line-height: 60px;
+}
+
+li {


Use only class selector

volodymyr-soltys97

Hi, the demo link still not fixed from the previous review, you need to make a deploy again

volodymyr-soltys97 · 2024-09-15T20:04:34Z

src/style.css

+}
+
+.nav__link:hover {
+  color: var(--link-color) !important;


Don't use important

Suggested change

color: var(--link-color) !important;

color: var(--link-color);

volodymyr-soltys97 · 2024-09-15T20:04:47Z

src/style.css

+.nav-item:nth-of-type(1) .nav__link {
+  color: var(--link-color) !important;
+}


Suggested change

.nav-item:nth-of-type(1) .nav__link {

color: var(--link-color) !important;

}

.nav__link {

color: var(--link-color);

}

volodymyr-soltys97 · 2024-09-15T20:04:56Z

src/style.css

+  color: var(--link-color) !important;
+}
+
+.nav__link[data-qa='hover']:hover {


Suggested change

.nav__link[data-qa='hover']:hover {

.nav__link:hover {

volodymyr-soltys97 · 2024-09-15T20:05:36Z

src/style.css

+.is-active:hover::after {
+  transform: scaleX(1);
+}
+
+.is-active:link {
+  color: black;
+}
+
+.is-active:visited {
+  color: grey;
+}
+
+.is-active:hover {
+  text-decoration-color: var(--link-color);
+}
+
+.nav-item.is-active.apple-active .nav__link {
+  color: var(--link-color);
+}


Redundant

Suggested change

.is-active:hover::after {

transform: scaleX(1);

}

.is-active:link {

color: black;

}

.is-active:visited {

color: grey;

}

.is-active:hover {

text-decoration-color: var(--link-color);

}

.nav-item.is-active.apple-active .nav__link {

color: var(--link-color);

}

volodymyr-soltys97

Hi, the demo link still not fixed, you need to make a deploy after your changes, if you need help feel free in the fe_chat

volodymyr-soltys97 · 2024-09-16T06:14:35Z

src/index.html

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


Suggested change

</a>

<nav class="nav">

</a>

<nav class="nav">

volodymyr-soltys97 · 2024-09-16T06:14:58Z

src/style.css

+  align-items: center;
+}
+
+img {


Use class selector for image

you yourself asked to remove class nav-logo

volodymyr-soltys97 · 2024-09-16T06:15:14Z

src/style.css

+.nav__link[href='#apple'] {
+  color: var(--link-color);
+}


Redundant

Suggested change

.nav__link[href='#apple'] {

color: var(--link-color);

}

can't do without .nav__link[href='#apple']

and now it should work

volodymyr-soltys97 · 2024-09-16T06:15:55Z

src/style.css

+  transform: scaleX(1);
+  transition: transform 0.15s ease-in-out;


Redundant

Suggested change

transform: scaleX(1);

transition: transform 0.15s ease-in-out;

artuom2283 · 2024-09-16T07:01:29Z

I have passed all the tests, what else do I need?

etojeDenys

don't hesitate to ask your questions in the chat

etojeDenys · 2024-09-16T07:07:21Z

src/style.css

+  margin-left: 0;
+}
+
+.nav .nav__link {


Suggested change

.nav .nav__link {

.nav__link {

etojeDenys · 2024-09-16T07:07:40Z

src/style.css

+.nav__link[href='#apple'] {
+  color: var(--link-color);
+}


and now it should work

artuom2283 · 2024-09-16T07:16:33Z

did everything you said)

lerastarynets

You've got this, keep pushing forward!

artuom2283 added 2 commits September 14, 2024 20:06

add task solution

cc960ec

style fix

d9149a3

mateacademy-ai-mentor suggested changes Sep 14, 2024

View reviewed changes

add task solution

36d3f03

artuom2283 requested a review from mateacademy-ai-mentor September 14, 2024 18:19

fix uppercase

f6a5963

artuom2283 closed this Sep 15, 2024

artuom2283 reopened this Sep 15, 2024

vadiimvooo suggested changes Sep 15, 2024

View reviewed changes

fixed the mistakes

8efa107

artuom2283 requested a review from vadiimvooo September 15, 2024 17:53

volodymyr-soltys97 suggested changes Sep 15, 2024

View reviewed changes

fixed the mistakes

3071fea

artuom2283 requested a review from volodymyr-soltys97 September 15, 2024 19:52

volodymyr-soltys97 suggested changes Sep 15, 2024

View reviewed changes

fixed the mistakes 2.0

da9ecb3

artuom2283 requested a review from volodymyr-soltys97 September 16, 2024 05:19

volodymyr-soltys97 suggested changes Sep 16, 2024

View reviewed changes

fixed the mistakes 3.0

6240ce3

artuom2283 requested a review from volodymyr-soltys97 September 16, 2024 07:01

etojeDenys suggested changes Sep 16, 2024

View reviewed changes

fixed the mistakes 4.0

b1b83a5

artuom2283 requested a review from etojeDenys September 16, 2024 07:14

lerastarynets approved these changes Sep 16, 2024

View reviewed changes

	<li class="nav-item is-active apple-active">
	<li class="nav-item">

	<a href="#smartphones">smartphones</a>
	<a class="nav__link" href="#smartphones">smartphones</a>

	color: var(--link-color) !important;
	color: var(--link-color);

		transform: scaleX(1);
		transition: transform 0.15s ease-in-out;

Develop #5361

Are you sure you want to change the base?

Develop #5361

Conversation

artuom2283 commented Sep 14, 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

vadiimvooo 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

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

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

artuom2283 commented Sep 16, 2024

etojeDenys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artuom2283 commented Sep 16, 2024

lerastarynets left a comment

Choose a reason for hiding this comment