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

need to adjust the rectangle #5664

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

Conversation

Sahardi
Copy link

@Sahardi Sahardi commented Nov 9, 2024

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! However, there are a few critical issues that need to be addressed before we can approve the pull request. 🚀

  1. HTML Structure: The use of <ul> elements is incorrect for navigation. You should use <li> elements within a <ul> or <nav> and wrap the text in <a> tags with href attributes for proper navigation structure.

  2. Font Loading: The @font-face rule in your CSS is pointing to a Google Fonts page URL instead of a valid font file URL. Make sure to use the correct URL format provided by Google Fonts to ensure the Roboto font loads correctly.

  3. HTML Semantics: The <div> element with a src attribute is invalid. If you need to display an image, use an <img> tag instead.

These issues affect the core functionality and requirements of the task. Once these are resolved, your code will be much closer to meeting the task requirements. Keep up the great work, and don't hesitate to reach out if you need further assistance! 😊

✨ 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
Comment on lines 27 to 29
<div src="./images/Rectangle 2.svg"
class="rect"
alt="line">>

Choose a reason for hiding this comment

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

The <div> element should not have a src attribute. If you intend to display an image, use an <img> tag instead.

src/index.html Outdated
Comment on lines 30 to 32
<ul class="apple is-active"
href="apple.html">APPLE
</ul>

Choose a reason for hiding this comment

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

The <ul> element is incorrectly used here. If you intend to create a navigation link, consider using an <li> element within a <ul> or <nav> and wrap the text in an <a> tag with the href attribute.

src/index.html Outdated
Comment on lines 34 to 36
<ul class="sams"
href="samsung.html">SAMSUNG
</ul>

Choose a reason for hiding this comment

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

Similar to the previous comment, the <ul> element is not appropriate here. Use an <li> element within a <ul> or <nav> and wrap the text in an <a> tag.

src/index.html Outdated
Comment on lines 38 to 40
<ul class="smart"
href="smartphones.html">SMARTPHONES
</ul>

Choose a reason for hiding this comment

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

The <ul> element is incorrectly used for navigation. Replace it with an <li> element and wrap the text in an <a> tag.

src/index.html Outdated
Comment on lines 42 to 45
<ul class="lapcom"
data-qa="hover"
href="laptops&computers.html">LAPTOPS & COMPUTERS
</ul>

Choose a reason for hiding this comment

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

The <ul> element should be replaced with an <li> element for navigation purposes, and the text should be wrapped in an <a> tag.

src/index.html Outdated
Comment on lines 47 to 49
<ul class="gadg"
href="gadgets.html">GADGETS
</ul>

Choose a reason for hiding this comment

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

Replace the <ul> element with an <li> element and wrap the text in an <a> tag for proper navigation structure.

src/index.html Outdated
Comment on lines 51 to 53
<ul class="tab"
href="tablets.html">TABLETS
</ul>

Choose a reason for hiding this comment

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

The <ul> element is not suitable here. Use an <li> element and wrap the text in an <a> tag for navigation.

src/index.html Outdated
Comment on lines 55 to 57
<ul class="ph"
href="photo.html">PHOTO
</ul>

Choose a reason for hiding this comment

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

Replace the <ul> element with an <li> element and wrap the text in an <a> tag for proper navigation.

src/index.html Outdated
Comment on lines 59 to 61
<ul class="vi"
href="video.html">VIDEO
</ul>

Choose a reason for hiding this comment

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

The <ul> element should be replaced with an <li> element, and the text should be wrapped in an <a> tag for navigation.

src/style.css Outdated
Comment on lines 5 to 8
@font-face {
font-family: Roboto;
src: url(https://fonts.google.com/specimen/Roboto);
}

Choose a reason for hiding this comment

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

The src attribute in the @font-face rule should point to a valid font file URL, such as a .woff, .woff2, or similar file type, rather than a Google Fonts page URL. Consider using the correct URL format provided by Google Fonts for embedding fonts.

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