Develop #5380
Develop #5380
Conversation
Krrampuss
commented
Sep 18, 2024
Krrampuss
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Your progress is commendable! 🌟 While the Pull Request has been approved, please take a moment to address the points mentioned in the comments. These include using fallback fonts, selecting elements appropriately in CSS, avoiding unnecessary classes, being cautious with the universal selector for performance reasons, formatting href attributes correctly, providing descriptive alt text for images, and keeping your HTML attributes neatly formatted. Keep up the good work, and use these tips to enhance your code in the future! 💪 Remember, refining your code is a continuous journey. Happy coding! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <html | ||
| class="html" | ||
| lang="en" |
There was a problem hiding this comment.
The class attribute on the html tag is not necessary unless it's being used for a specific purpose in CSS or JavaScript. If it's not used, it's better to remove it to keep the markup clean.
| <a href="#"> | ||
| <img | ||
| class="img" | ||
| src="images/logo.png" | ||
| alt="MOYO_main_logo" |
There was a problem hiding this comment.
The href attribute in the anchor tag should not be empty or contain just '#'. If this is a placeholder, consider adding a comment or using a more descriptive placeholder. Also, the alt attribute should describe the image content, 'MOYO_main_logo' could be more descriptive to improve accessibility and SEO.
| <a | ||
| class="nav__link is-active" | ||
| href="#" | ||
| > |
There was a problem hiding this comment.
Avoid using spaces in the href attribute of an anchor tag. Anchor links should start with the # symbol followed by a meaningful identifier corresponding to an id of an element on the page.
| * { | ||
| margin: 0; | ||
| padding: 0; | ||
| } |
There was a problem hiding this comment.
Don't use * selector (it impacts performance). Set styles only for elements that require them. Instead, you can set margin: 0 and padding: 0 in the body tag if you need to reset the default browser styling.
| .html { | ||
| --header-link-color: #00acdc; | ||
| } |
There was a problem hiding this comment.
It's not common practice to create a class named .html. If you want to define CSS variables or set styles for the entire document, you should place them within the :root selector.
| .body { | ||
| margin: 0; | ||
| } |
There was a problem hiding this comment.
Similar to the .html class, it is unusual to have a .body class. If you are trying to style the <body> element, you should use the body tag selector.
| .img { | ||
| display: block; | ||
| } |
There was a problem hiding this comment.
Avoid using tag names for styling like .img. If you need to apply a style to all <img> elements, it's better to use the img tag directly, unless there's a specific reason to use a class.
| justify-content: space-between; | ||
| align-items: center; | ||
| padding: 0 50px; | ||
| font-family: Roboto, sans-serif; |
There was a problem hiding this comment.
Remember to use fallback fonts - alternative font-family in case the main one doesn't work. For example, font-family: Roboto, Arial, sans-serif;.
