add task solution #5352
add task solution #5352
Conversation
Krrampuss
commented
Sep 11, 2024
Krrampuss
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Hey there! 👋 It looks like there are a few important aspects to address in your code. Remember to use semantic tags correctly, provide meaningful class names, and ensure proper indentation for better readability. Also, don't forget to include fallback fonts and avoid using the * selector for performance reasons. Keep up the good work, and with a bit of polishing, your code will be top-notch! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| href="https://fonts.googleapis.com/css2?family=Roboto:wght@400;500;700&display=swap" | ||
| rel="stylesheet" | ||
| /> | ||
| > |
There was a problem hiding this comment.
There is an extraneous '>' character at the end of line 17 which should be removed.
src/index.html
Outdated
| <body> | ||
| <h1>Moyo header</h1> | ||
| </body> | ||
| <header> |
There was a problem hiding this comment.
The 'header' element should be inside the 'body' tag, not directly under the 'html' tag.
src/index.html
Outdated
| src="images/logo.png" | ||
| alt="logo" | ||
| /> | ||
| </a> |
There was a problem hiding this comment.
The 'alt' attribute for images should describe the image content, not just say 'logo'. For example, 'Company Logo' would be more descriptive.
src/index.html
Outdated
| /> | ||
| </a> | ||
| <nav> | ||
| <ul> |
There was a problem hiding this comment.
Add empty lines between multiline sibling blocks of HTML to improve readability. There should be an empty line before and after the 'ul' element.
src/index.html
Outdated
| Apple | ||
| </a> | ||
| </li> | ||
| <li><a href="#">Samsung</a></li> |
There was a problem hiding this comment.
Remember about correct indentation between parent and child elements. The 'a' tags within the 'li' elements should be indented.
src/index.html
Outdated
| <li><a href="#">Tablets</a></li> | ||
| <li><a href="#">Photo</a></li> | ||
| <li><a href="#">Video</a></li> | ||
| </ul> |
There was a problem hiding this comment.
Class names represent the meaning of the content. Instead of generic classes like 'is-active', use more descriptive class names that represent the content or the role of the element, such as 'nav__link--active'.
src/style.css
Outdated
| * { | ||
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
Don't use the * selector as it impacts performance. It is better to set styles only for elements that require them.
src/style.css
Outdated
| header { | ||
| width: 100%; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: space-between; | ||
| background-color: #fff; | ||
| margin: 0; | ||
| font-family: Roboto, sans-serif; | ||
| font-size: 12px; | ||
| font-weight: 500; | ||
| line-height: 60px; | ||
| text-transform: uppercase; |
There was a problem hiding this comment.
Avoid fixing container sizes such as width: 100%; on the header unless there is a specific requirement for it. Let the content define the size to prevent overflow or accidental scroll bars.
src/style.css
Outdated
| margin: 0; | ||
| font-family: Roboto, sans-serif; |
There was a problem hiding this comment.
Remember to use fallback fonts in the font-family property to ensure text is displayed with a default font in case the primary font fails to load.
src/style.css
Outdated
| ul { | ||
| display: flex; | ||
| align-items: center; | ||
| list-style: none; |
There was a problem hiding this comment.
Don't use tag names for styling. Instead, use classes to style your elements. For example, apply a class to the ul element and style that class.
There was a problem hiding this comment.
Good work 👍
Don't forget about the checklisst.
Also, fix your demo and test report links in PR as they point to the repository itself instead
