task solution #5381
task solution #5381
Conversation
D0dgerJ
commented
Sep 18, 2024
D0dgerJ
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
You've done a fantastic job on this Pull Request! 👍 However, there are a few issues with href attributes containing spaces, and some best practices that we can improve upon, such as using fallback fonts, avoiding the * selector for better performance, and ensuring consistent indentation. Fixing these will make your code even more robust and maintainable! Let's make those changes 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! ✨
src/index.html
Outdated
| <header class="header"> | ||
| <a | ||
| href="/" | ||
| class="logo-link" | ||
| > | ||
| <img | ||
| src="images/logo.png" | ||
| alt="MOYO Logo" | ||
| class="logo" | ||
| /> |
There was a problem hiding this comment.
There should be an empty line between multiline sibling blocks of HTML. Please add an empty line after the closing </a> tag and before the <nav> tag to improve readability.
src/index.html
Outdated
| <img | ||
| src="images/logo.png" | ||
| alt="MOYO Logo" | ||
| class="logo" | ||
| /> |
There was a problem hiding this comment.
Ensure that the alt attribute of the <img> tag describes the content of the image. 'MOYO Logo' is a good description, but if 'MOYO' has a specific meaning or is a brand, including that context could be beneficial (e.g., 'MOYO Electronics Brand Logo').
src/index.html
Outdated
| <a | ||
| href="#" | ||
| class="nav__link is-active" | ||
| > | ||
| Apple |
There was a problem hiding this comment.
Avoid using spaces in the href attribute of the <a> tag. If this is meant to be a placeholder, consider using '#apple' to indicate the intended target section.
src/index.html
Outdated
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Samsung |
There was a problem hiding this comment.
The href attribute should not contain spaces. Use '#samsung' as the value if this link is meant to direct to a Samsung section.
src/index.html
Outdated
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Smartphones |
There was a problem hiding this comment.
Similar to the previous comments, ensure the href attribute does not contain spaces. Use '#smartphones' for better clarity.
src/index.html
Outdated
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Photo |
There was a problem hiding this comment.
The href attribute should be free of spaces. Use '#photo' as the href value.
src/index.html
Outdated
| <a | ||
| href="#" | ||
| class="nav__link" | ||
| > | ||
| Video |
There was a problem hiding this comment.
The href attribute should not contain spaces. Use '#video' as the href value.
src/style.css
Outdated
| * { | ||
| margin: 0; | ||
| padding: 0; | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
Don't use * selector as it impacts performance. It's better to set styles only for elements that require them. If you need to reset default margins and paddings, consider using a more specific reset CSS or applying these styles only to the elements that need them.
src/style.css
Outdated
| } | ||
|
|
||
| html { | ||
| 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 instance, you could specify a generic font family like 'sans-serif' as a fallback.
| } | ||
|
|
||
| .nav__item { | ||
| margin-right: 20px; |
There was a problem hiding this comment.
Be consistent with your vertical margins. It's recommended to add only top or only bottom margin, but not both, to prevent issues with margin collapse. In this case, consider using only margin-bottom for .nav__item.
There was a problem hiding this comment.
Great job on the Pull Request! Just a few minor points to consider for the future: remember to maintain consistent vertical margins, use fallback fonts, avoid styling with tag names, and watch out for spaces in href attributes. Also, keep your alt text clean and ensure consistent indentation in your code. These small tweaks can make your code even better! Keep up the good work! 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| > | ||
| <img | ||
| src="images/logo.png" | ||
| alt="MOYO Electronics Brand Logo'" |
There was a problem hiding this comment.
The alt attribute should describe the image content, but avoid using special characters like single quotes at the end unless they are part of the actual description. It should be alt="MOYO Electronics Brand Logo".
| <a | ||
| href="#laptops-computers" | ||
| class="nav__link" | ||
| data-qa="hover" |
There was a problem hiding this comment.
Don't use spaces in <a> tag's href property. The href for 'Laptops & Computers' contains a space, which should be replaced with a hyphen or underscore (e.g., href="#laptops-computers").
| ul, | ||
| ol { | ||
| padding: 0; | ||
| } |
There was a problem hiding this comment.
Don't use tag names for styling (except html and body). Style elements using .class. In your case, you could create a class like .list-reset and apply it to ul or ol where necessary.
| body, | ||
| h1, | ||
| h2, | ||
| h3, | ||
| h4, | ||
| p, | ||
| ul, | ||
| ol, | ||
| li, | ||
| figure, | ||
| figcaption, | ||
| blockquote, | ||
| dl, | ||
| dd { | ||
| margin: 0; | ||
| list-style: none; | ||
| } |
There was a problem hiding this comment.
Avoid using tag names for styling. It's better to use classes to avoid unintended side effects on other elements of the same type. For example, you might not want all p tags to have no margin in every context.
| } | ||
|
|
||
| html { | ||
| font-family: Roboto, Arial, 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 instance, 'Roboto', 'Arial', sans-serif could be specified as 'Roboto', 'Arial', 'Helvetica', sans-serif.
| .nav__item { | ||
| margin-right: 20px; | ||
| } | ||
|
|
||
| .nav__item:last-child { | ||
| margin-right: 0; | ||
| } |
There was a problem hiding this comment.
Be consistent with your vertical margins. You're adding right margin to .nav__item which is horizontal, but make sure you apply the same principle to vertical margins as well, by adding only top or only bottom margin but not both.
