Task solution(develop) #6105

aholubko · 2025-02-09T01:44:34Z

mateacademy-ai-mentor

Thank you for your hard work on this project! 😊 There are a few critical issues that need to be addressed before we can approve the pull request. The href attribute for the "Laptops & Computers" link contains a space, which is not valid in URLs and affects the functionality. Additionally, the CSS file specifies the font family incorrectly, which does not meet the task requirements. Please make these changes to ensure your code meets the core requirements. Once these are resolved, your work will be in great shape! Keep up the great effort! 🚀

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

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

mateacademy-ai-mentor · 2025-02-09T01:45:25Z

src/index.html

+          <li><a href="#samsung">SAMSUNG</a></li>
+          <li><a href="#smartphones">SMARTPHONES</a></li>
+          <li data-qa="hover">
+            <a href="#laptops & Computers">LAPTOP & COMPUTERS</a>


The 'href' attribute value '#laptops & Computers' contains a space, which is not valid in URLs. Consider replacing the space with '%20' or using a hyphen or underscore.

mateacademy-ai-mentor · 2025-02-09T01:45:25Z

src/index.html

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


Consider providing a more descriptive 'alt' attribute for the logo image to improve accessibility. For example, 'Company Logo' or 'Moyo Logo'.

mateacademy-ai-mentor · 2025-02-09T01:45:25Z

src/style.css

+  font-optical-sizing: auto;
+  font-weight: 500;
+  font-style: normal;
+  font-variation-settings: 'wdth' 100;


The 'font-variation-settings' property may not be supported in all browsers. Consider providing a fallback font style for better cross-browser compatibility.

mateacademy-ai-mentor · 2025-02-09T01:45:25Z

src/style.css

+
+.header {
+  font-family: Roboto, serif;
+  font-optical-sizing: auto;


The 'font-optical-sizing' property may not be supported in all browsers. Consider providing a fallback font size or style for better cross-browser compatibility.

mateacademy-ai-mentor · 2025-02-09T01:45:25Z

src/style.css

+  font-variation-settings: 'wdth' 100;
+  color: black;
+  height: 60px;
+  font-size: 12.3px;


The 'font-size' is set to 12.3px, which is an uncommon value. Consider using a standard font size like 12px or 14px for consistency.

mateacademy-ai-mentor

Great job on addressing the previous issues! 🎉 The HTML and CSS files have been updated to meet the task requirements. The href attribute for the "Laptops & Computers" link is now correctly formatted, and the font family in the CSS is specified as "Roboto" with appropriate fallbacks.

While the code meets the task requirements, remember to review the comments to further improve your code. Addressing any critical issues in the future will help you avoid similar problems and enhance your learning experience. Keep up the good work and continue to learn from your mistakes! 🚀

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

volodymyr-soltys97

Good job 👍
Let's improve your code

The default color for first link should be blue

The height for links should be 60px

Remove these margins and add padding right for the header

volodymyr-soltys97 · 2025-02-09T07:17:14Z

src/index.html

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


Add classes everywhere

Suggested change

<nav class="nav">

<ul>

<li>

<a

href="#apple"

class="is-active"

<nav class="nav">

<ul class="nav__list">

<li class="nav__item">

<a

href="#apple"

class="nav__link is-active"

volodymyr-soltys97 · 2025-02-09T07:17:58Z

src/index.html

+              APPLE
+            </a>
+          </li>
+          <li><a href="#samsung">SAMSUNG</a></li>


Fix code styles and add classes everywhere

Suggested change

<li><a href="#samsung">SAMSUNG</a></li>

<li class="nav__item">

<a href="#samsung" class="nav__link">SAMSUNG</a>

</li>

volodymyr-soltys97 · 2025-02-09T07:18:25Z

src/style.css

+  position: relative;
+}
+
+.nav a.is-active::after {


Suggested change

.nav a.is-active::after {

.is-active::after {

volodymyr-soltys97 · 2025-02-09T07:18:44Z

src/style.css

+}
+
+.nav a:hover {
+  color: #00acdc;


Create a variable for repeated color and use it everywhere

volodymyr-soltys97

Hi, the demo link hasn't styles, you need to make a deploy and check after the demo

volodymyr-soltys97 · 2025-02-10T06:29:44Z

src/style.css

+  margin-left: 50px;
+}
+
+.nav a:hover {


Suggested change

.nav a:hover {

.nav__link:hover {

vadiimvooo

Seems like there are still problems with your deploy. Please feel free to ask for help in fe_chat.

vadiimvooo · 2025-02-10T17:51:12Z

src/style.css

+  align-items: center;
+  margin: 0;
+  padding: 0;
+  height: 60px;


This height should be setted on link instead of list.

vadiimvooo · 2025-02-10T17:51:41Z

src/style.css

+  display: flex;
+  width: 40px;
+  height: 40px;
+  margin-left: 50px;


horizontal margin should be placed on header as padding

volodymyr-soltys97

Hi, the demo link still not fixed pls feel free in the fe_chat for help

add task solution

03cfddd

mateacademy-ai-mentor suggested changes Feb 9, 2025

View reviewed changes

fixed mistakes

2fa3cf5

aholubko requested a review from mateacademy-ai-mentor February 9, 2025 02:05

mateacademy-ai-mentor approved these changes Feb 9, 2025

View reviewed changes

volodymyr-soltys97 suggested changes Feb 9, 2025

View reviewed changes

fixed the mistakes that pointed out to me by mentor

4d1b396

aholubko requested a review from volodymyr-soltys97 February 10, 2025 04:28

volodymyr-soltys97 suggested changes Feb 10, 2025

View reviewed changes

src/style.css Outdated

margin-left: 50px;

}

.nav a:hover {

Copy link

volodymyr-soltys97 Feb 10, 2025

Choose a reason for hiding this comment

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

Suggested change

.nav a:hover {

.nav__link:hover {

fixed class name

a82cf66

aholubko requested a review from volodymyr-soltys97 February 10, 2025 17:12

vadiimvooo suggested changes Feb 10, 2025

View reviewed changes

Update styles

5192d4f

aholubko requested a review from vadiimvooo February 12, 2025 20:59

volodymyr-soltys97 suggested changes Feb 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Task solution(develop) #6105

Task solution(develop) #6105

aholubko commented Feb 9, 2025

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Feb 9, 2025

mateacademy-ai-mentor Feb 9, 2025

mateacademy-ai-mentor Feb 9, 2025

mateacademy-ai-mentor Feb 9, 2025

mateacademy-ai-mentor Feb 9, 2025

mateacademy-ai-mentor left a comment

volodymyr-soltys97 left a comment

volodymyr-soltys97 Feb 9, 2025

volodymyr-soltys97 Feb 9, 2025

volodymyr-soltys97 Feb 9, 2025

volodymyr-soltys97 Feb 9, 2025

volodymyr-soltys97 left a comment

volodymyr-soltys97 Feb 10, 2025

vadiimvooo left a comment

vadiimvooo Feb 10, 2025

vadiimvooo Feb 10, 2025

volodymyr-soltys97 left a comment

-          <li><a href="#samsung">SAMSUNG</a></li>
+          <li class="nav__item">
+            <a href="#samsung" class="nav__link">SAMSUNG</a>
+          </li>

Task solution(develop) #6105

Are you sure you want to change the base?

Task solution(develop) #6105

Conversation

aholubko commented Feb 9, 2025

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

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

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

volodymyr-soltys97 left a comment

Choose a reason for hiding this comment