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

Master #5874

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

Master #5874

wants to merge 2 commits into from

Conversation

This-is-Lapwing
Copy 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.


Hello! 😊 It looks like there are a few critical issues that need to be addressed before we can approve the pull request. Here are the key points to focus on:

  1. CSS Selector Usage: The task requires using .class selectors instead of tag names like ul and a. This is important for maintaining a scalable and maintainable codebase.

  2. CSS Fallback Fonts: Ensure that you include fallback fonts in your font-family property to maintain a consistent appearance if the primary font fails to load.

  3. CSS Variable for Blue Color: Implement a CSS variable for the blue color as specified in the task requirements. This will help in maintaining consistency and ease of updates.

  4. HTML Formatting: Consistent indentation is crucial for readability. Please ensure your HTML uses 2 spaces for indentation.

  5. Semantic Tags: Replace non-semantic tags like div with semantic tags where possible to improve the accessibility and readability of your HTML.

  6. CSS for Uppercase Letters: Make sure to use CSS to display all letters in uppercase as required.

  7. CSS for Vertical Centering and Header Height: Address the requirement to vertically center content and set header height in one place for better layout control.

  8. CSS Pseudo-element for Active Link: Use a CSS variable for the color of the pseudo-element for the active link.

  9. CSS for Link Hover: Ensure the hover color uses the CSS variable for consistency.

  10. CSS Indentation and Line Length: Keep your CSS lines within 80 characters for better readability.

Please make these changes, and feel free to reach out if you need any help. Your effort is appreciated, and you're on the right track! Keep going! 🚀

✨ 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
<link
rel="preconnect"
href="https://fonts.gstatic.com"
crossorigin="use-credentials"

Choose a reason for hiding this comment

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

The crossorigin attribute should be set to anonymous instead of use-credentials for Google Fonts to avoid potential CORS issues. The use-credentials value is not typically used for fonts and may cause unnecessary complications.

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 progress! 🎉 While there are still some areas for improvement, such as using class selectors instead of tag names in your CSS and implementing CSS variables for colors, your efforts are commendable. Remember to address these issues to enhance the maintainability and consistency of your code. 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! ✨

alt="moyo__logo"
/>
</a>
<div class="nav_block">

Choose a reason for hiding this comment

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

Consider using a <nav> tag directly instead of wrapping it in a <div> for better semantic structure.

margin: 0;
}

ul {

Choose a reason for hiding this comment

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

Avoid using tag selectors like ul. Use class selectors instead to improve maintainability.

list-style: none;
}

a {

Choose a reason for hiding this comment

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

Avoid using tag selectors like a. Use class selectors instead to improve maintainability.

@@ -1,3 +1,69 @@
body {
font-family: Roboto, sans-serif;

Choose a reason for hiding this comment

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

Consider adding a fallback font to ensure consistent appearance if Roboto fails to load.

}

.nav__link:hover {
color: #00acdc;

Choose a reason for hiding this comment

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

Use a CSS variable for the hover color to maintain consistency and ease of updates.

Comment on lines +47 to +48
color: #00acdc;
box-shadow: 0 4px 0 #00acdc;

Choose a reason for hiding this comment

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

Use a CSS variable for the active color to maintain consistency and ease of updates.

Comment on lines +53 to +59
color: #00acdc;
}

.is-active::after {
content: '';
position: absolute;
background-color: #00acdc;

Choose a reason for hiding this comment

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

Use a CSS variable for the color of the pseudo-element for the active link to maintain consistency.

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