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 #5857

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

Develop #5857

wants to merge 3 commits into from

Conversation

srvalle
Copy link

@srvalle srvalle commented Dec 23, 2024

  • DEMO LINK

  • TEST REPORT LINK

  • Header height is set in 1 place (for the links)

  • Content is vertically centered (for any header height)

  • CSS is used to show all letters in Uppercase (don't type them in HTML)

  • Logo is an image wrapped with a link

  • CSS Variable is used for a blue color

  • Pseudo-element is used for a blue line below the active link

  • Code follows all the Code Style Rules ❗️

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:

  1. Multiple Active Links: The presence of multiple elements with the is-active class could lead to confusion or unexpected behavior, especially if this class is meant to highlight a single active item. Please ensure that only one element uses this class at a time.

  2. Pseudo-element Positioning: The .is-active::after pseudo-element uses position: absolute, but it may not display correctly unless its parent element is positioned. Make sure the parent element has a position property other than static to avoid layout issues.

  3. Logo Flex Display: The .logo class uses display: flex, which is not typically applied to images. Consider removing this property unless there's a specific reason for its use.

  4. Accessibility of Links: Using href="#" is not ideal for accessibility and usability. Consider using a more meaningful URL or a JavaScript function to handle the click event if this is a placeholder.

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 have any questions or 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 Show resolved Hide resolved
src/index.html Outdated Show resolved Hide resolved
src/index.html Outdated Show resolved Hide resolved
src/index.html Outdated
<ul class="list">
<li class="list-item">
<a
href="#"

Choose a reason for hiding this comment

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

Using href="#" is not ideal for accessibility and usability. Consider using a more meaningful URL or a JavaScript function to handle the click event if this is a placeholder.

src/index.html Outdated
</li>
<li class="list-item">
<a
href="#"

Choose a reason for hiding this comment

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

Using href="#" is not ideal for accessibility and usability. Consider using a more meaningful URL or a JavaScript function to handle the click event if this is a placeholder.

}

.list .is-active::after {
position: absolute;

Choose a reason for hiding this comment

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

The .is-active::after pseudo-element uses position: absolute, but it may not display correctly unless its parent element is positioned (i.e., has a position property other than static). Ensure the parent element is positioned to avoid layout issues.

src/style.css Outdated
.logo {
width: 40px;
height: 40px;
display: flex;

Choose a reason for hiding this comment

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

The .logo class uses display: flex, which is not typically applied to images. Consider removing this property unless there is a specific reason for its use.

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.

Great job on your submission! 🎉 While your code is mostly well-structured and follows standard practices, there are a few critical issues that need attention:

  1. Multiple Active Links: Ensure only one element has the is-active class at a time to prevent confusion or unexpected behavior.
  2. Pseudo-element Positioning: The .is-active::after pseudo-element is positioned absolutely. Make sure its parent element has a position property other than static to avoid layout issues.
  3. Accessibility of Links: The href attributes for the links are either missing or set to "#". Consider using meaningful URLs or handling the click event with JavaScript for better accessibility.

These improvements will enhance the functionality and accessibility of your code. Keep up the good work, and make sure to address these points in future projects to continue growing your skills! 🚀

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

<nav class="nav">
<ul class="list">
<li class="list-item">
<a class="item-link is-active">

Choose a reason for hiding this comment

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

The is-active class is applied here. Ensure that only one element has this class at a time to avoid confusion or unexpected behavior.

<nav class="nav">
<ul class="list">
<li class="list-item">
<a class="item-link is-active">

Choose a reason for hiding this comment

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

The href attribute is missing a URL. Consider using a meaningful URL or a JavaScript function to handle the click event for better accessibility and usability.

Comment on lines +66 to +75
.list .is-active::after {
position: absolute;
content: "";
width: 36px;
height: 4px;
left: 0;
bottom: 0;
background: var(--blue-color);
border-radius: 8px;
}

Choose a reason for hiding this comment

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

The .is-active::after pseudo-element uses position: absolute. Ensure that its parent element has a position property other than static to avoid layout issues. Consider adding position: relative to the parent element.

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