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.

Sign up for GitHub

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

Jump to bottom

add task solution #5823

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

add task solution #5823

wants to merge 2 commits into from

Conversation

wiltonmartinsdev
Copy link

mateacademy-ai-mentor
mateacademy-ai-mentor suggested changes Dec 16, 2024
View reviewed changes
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! 😊 While your code meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request. Specifically, the use of # in the href attributes for multiple anchor tags is a significant issue as it affects navigation functionality. Additionally, the CSS variable for the blue color needs to be implemented as per the task requirements. Please address these points, and you'll be all set! Keep up the great work! 🚀

✨ 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 36 to 44
<a
href="#"
class="logo"
>
<img
src="./images/logo.png"
alt="logotipo"
/>
</a>
Copy link

Choose a reason for hiding this comment

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

The 'href' attribute in the anchor tag for the logo is set to '#', which means it currently doesn't link to any page. Consider updating it to point to the homepage or a relevant section.

src/index.html Outdated
Comment on lines 48 to 53
<a
href="#"
class="is-active"
>
apple
</a>
Copy link

Choose a reason for hiding this comment

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

The 'href' attribute in this anchor tag is set to '#', which means it doesn't link to any specific page. Ensure to update it with the correct URL or section ID.

src/index.html Outdated
apple
</a>
</li>
<li><a href="#">samsung</a></li>
Copy link

Choose a reason for hiding this comment

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

The 'href' attribute in this anchor tag is set to '#', which means it doesn't link to any specific page. Ensure to update it with the correct URL or section ID.

src/index.html Outdated
</a>
</li>
<li><a href="#">samsung</a></li>
<li><a href="#">smartphones</a></li>
Copy link

Choose a reason for hiding this comment

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

The 'href' attribute in this anchor tag is set to '#', which means it doesn't link to any specific page. Ensure to update it with the correct URL or section ID.

src/index.html Outdated
Comment on lines 58 to 63
<a
href="#"
data-qa="hover"
>
laptops & computers
</a>
Copy link

Choose a reason for hiding this comment

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

The 'href' attribute in this anchor tag is set to '#', which means it doesn't link to any specific page. Ensure to update it with the correct URL or section ID.

src/index.html Outdated
</li>
<li><a href="#">gadgets</a></li>
<li><a href="#">tablets</a></li>
<li><a href="#">photo</a></li>
Copy link

Choose a reason for hiding this comment

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

The 'href' attribute in this anchor tag is set to '#', which means it doesn't link to any specific page. Ensure to update it with the correct URL or section ID.

src/index.html Outdated
Comment on lines 68 to 70
<li>
<a href="#">video</a>
</li>
Copy link

Choose a reason for hiding this comment

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

The 'href' attribute in this anchor tag is set to '#', which means it doesn't link to any specific page. Ensure to update it with the correct URL or section ID.

src/style.css
@@ -1,3 +1,144 @@
body {
margin: 0;
font-family: Roboto, sans-serif;
Copy link

Choose a reason for hiding this comment

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

The font-family is set to 'Roboto', which is a web font. Ensure that the font is correctly linked in your HTML file to avoid fallback to the default sans-serif font.

src/style.css Outdated
Comment on lines 8 to 75
@media screen and (max-width: 1024px) {
header {
display: flex;
justify-content: space-between;
align-items: center;
padding: 0 50px;
}

header a.logo {
display: flex;
align-items: center;
}

ul {
display: flex;
list-style-type: none;
margin: 0;
padding: 0;
}

li {
margin-right: 20px;
}

li:first-child {
margin-left: 0;
}

li:last-child {
margin-right: 0;
}

a {
text-decoration: none;
text-transform: uppercase;
color: black;
display: flex;
align-items: center;
height: 60px;
position: relative;
}

a:hover {
color: #00acdc;
}

a.is-active {
position: relative;
}

a.is-active::after {
content: '';
display: block;
width: 100%;
height: 4px;
background-color: #00acdc;
position: absolute;
bottom: 0;
border-radius: 8px;
}

a.is-active,
a:active {
color: #00acdc;
border-bottom: none;
border-radius: 0;
}
}
Copy link

Choose a reason for hiding this comment

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

The media query for max-width: 1024px is used to apply styles for smaller screens. Ensure that the styles within this block are necessary and do not conflict with other styles.

src/style.css Outdated
Comment on lines 77 to 144
@media screen and (min-width: 1024px) {
header {
display: flex;
justify-content: space-between;
align-items: center;
padding: 0 50px;
}

header a.logo {
display: flex;
align-items: center;
}

ul {
display: flex;
list-style-type: none;
margin: 0;
padding: 0;
}

li {
margin-right: 20px;
}

li:first-child {
margin-left: 0;
}

li:last-child {
margin-right: 0;
}

a {
text-decoration: none;
text-transform: uppercase;
color: black;
display: flex;
align-items: center;
height: 60px;
position: relative;
}

a:hover {
color: #00acdc;
}

a.is-active {
position: relative;
}

a.is-active::after {
content: '';
display: block;
width: 100%;
height: 4px;
background-color: #00acdc;
position: absolute;
bottom: 0;
border-radius: 8px;
}

a.is-active,
a:active {
color: #00acdc;
border-bottom: none;
border-radius: 0;
}
}
Copy link

Choose a reason for hiding this comment

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

The media query for min-width: 1024px is used to apply styles for larger screens. Ensure that the styles within this block are necessary and do not conflict with other styles.

mateacademy-ai-mentor
mateacademy-ai-mentor approved these changes Dec 16, 2024
View reviewed changes
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 job on addressing the previous issues! 🎉 Your code now meets the task requirements effectively. Here are some highlights and suggestions:

  • CSS Variable for Blue Color: You've correctly implemented the CSS variable for the blue color, which is used consistently throughout the stylesheet. This aligns well with the task requirements.

  • href Attributes in Anchor Tags: The update to valid paths in the href attributes resolves the critical issue, enhancing navigation functionality.

  • Semantic Tags: Excellent use of semantic tags like <header>, <nav>, <ul>, <li>, and <a>, which improves the structure and accessibility of your HTML.

  • Formatting: Your CSS and HTML formatting is clean and follows the required guidelines, making the code easy to read and maintain.

  • Additional Requirements: You've successfully implemented other requirements, such as using the Roboto font and ensuring the header stretches the full page width.

While the current implementation is solid, ensure that all task requirements, such as the CSS variable for the blue color, are fully implemented in the corresponding CSS file. This will help you avoid similar issues in the future.

Keep up the great work, and continue to learn from each task. Your attention to detail and commitment to improving your code are commendable! 🚀

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mateacademy-ai-mentor mateacademy-ai-mentor mateacademy-ai-mentor approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@wiltonmartinsdev @mateacademy-ai-mentor