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.

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

Develop #5822

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Develop #5822

wants to merge 6 commits into from

Conversation

anastasiiadns
Copy link

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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


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 most important ones include ensuring all href attributes in your navigation links are valid URLs, and correcting the positioning of the ::after pseudo-element by setting position: relative; on the parent element. Additionally, the logo should be wrapped in an anchor tag to meet the requirements. Once these issues are resolved, your code will be in great shape! Keep up the great work, and don't hesitate to reach out if you need any help! 🚀

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

src/index.html Outdated
<ul>
<li>
<a
href="Apple"

Choose a reason for hiding this comment

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

The href attribute for the 'Apple' link should be a valid URL. Consider using a relative or absolute URL, such as href='/apple' or href='https://example.com/apple'.

src/index.html Outdated
Apple
</a>
</li>
<li><a href="Samsung">Samsung</a></li>

Choose a reason for hiding this comment

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

The href attribute for the 'Samsung' link should be a valid URL. Consider using a relative or absolute URL, such as href='/samsung' or href='https://example.com/samsung'.

src/index.html Outdated
</a>
</li>
<li><a href="Samsung">Samsung</a></li>
<li><a href="Smartphones">Smartphones</a></li>

Choose a reason for hiding this comment

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

The href attribute for the 'Smartphones' link should be a valid URL. Consider using a relative or absolute URL, such as href='/smartphones' or href='https://example.com/smartphones'.

src/index.html Outdated
<li><a href="Smartphones">Smartphones</a></li>
<li>
<a
href="Laptops&Computers"

Choose a reason for hiding this comment

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

The href attribute for the 'Laptops & Computers' link should be a valid URL. Consider using a relative or absolute URL, such as href='/laptops-computers' or href='https://example.com/laptops-computers'.

src/index.html Outdated
Laptops & Computers
</a>
</li>
<li><a href="Gadgets">Gadgets</a></li>

Choose a reason for hiding this comment

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

The href attribute for the 'Gadgets' link should be a valid URL. Consider using a relative or absolute URL, such as href='/gadgets' or href='https://example.com/gadgets'.

src/index.html Outdated
</a>
</li>
<li><a href="Gadgets">Gadgets</a></li>
<li><a href="Tablets">Tablets</a></li>

Choose a reason for hiding this comment

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

The href attribute for the 'Tablets' link should be a valid URL. Consider using a relative or absolute URL, such as href='/tablets' or href='https://example.com/tablets'.

src/index.html Outdated
</li>
<li><a href="Gadgets">Gadgets</a></li>
<li><a href="Tablets">Tablets</a></li>
<li><a href="Photo">Photo</a></li>

Choose a reason for hiding this comment

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

The href attribute for the 'Photo' link should be a valid URL. Consider using a relative or absolute URL, such as href='/photo' or href='https://example.com/photo'.

src/index.html Outdated
<li><a href="Gadgets">Gadgets</a></li>
<li><a href="Tablets">Tablets</a></li>
<li><a href="Photo">Photo</a></li>
<li><a href="Video">Video</a></li>

Choose a reason for hiding this comment

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

The href attribute for the 'Video' link should be a valid URL. Consider using a relative or absolute URL, such as href='/video' or href='https://example.com/video'.

Comment on lines +51 to +53
nav ul li a.is-active::after {
content: '';
position: absolute;

Choose a reason for hiding this comment

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

The ::after pseudo-element in the .is-active class uses position: absolute; but there is no position: relative; set on a parent element. This might cause the pseudo-element to be positioned incorrectly. Consider setting position: relative; on the parent element to ensure correct positioning.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Review Summary

Great effort on your submission! 🎉 You've made significant progress, but there are a few critical issues that need attention:

  1. Logo Wrapping: Ensure the logo is wrapped in an anchor tag to make it clickable, typically linking back to the homepage.

  2. Positioning of ::after Pseudo-element: The ::after pseudo-element for the active link is positioned absolutely. To ensure it is positioned correctly, the parent element should have position: relative;.

  3. Navigation Links href Attributes: The href attributes in the navigation links should be anchor links starting with the # symbol, as required by the task.

These issues are crucial for the functionality and user experience of your project. Please address them in future iterations to enhance your code quality. Keep up the good work and continue learning from these experiences! 🚀

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

Comment on lines +34 to +38
<img
src="images/logo.png"
alt="logo"
class="image"
/>

Choose a reason for hiding this comment

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

The logo image should be wrapped in an anchor tag to meet the task requirements. This will make the logo clickable and usually links back to the homepage.

Comment on lines +51 to +59
nav ul li a.is-active::after {
content: '';
position: absolute;
height: 4px;
width: 37px;
top: 56px;
background-color: #00acdc;
border-radius: 8px;
}

Choose a reason for hiding this comment

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

The ::after pseudo-element is positioned absolutely. To ensure it is positioned correctly relative to its parent, the parent element (in this case, the anchor tag) should have position: relative; applied. This will ensure the pseudo-element is positioned correctly under the active link.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants