add task solution #5368
add task solution #5368
Conversation
bodik-s4
commented
Sep 16, 2024
bodik-s4
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Great effort so far! 🌟 However, we need to address the missing href attributes in the <a> tags to ensure the links are functional. This is a critical part of the functionality, so please add valid references to these tags. Once that's done, we'll be all set! Keep up the good work, and we're almost there! 💪
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <header class="header"> | ||
| <a | ||
| class="header__link" | ||
| href="images/logo.png" |
There was a problem hiding this comment.
Don't use spaces in <a> tag's href property. It seems like you're linking to an image, which should be displayed using an <img> tag, not linked via <a> with href.
src/index.html
Outdated
| > | ||
| <img | ||
| src="images/logo.png" | ||
| alt="logo" |
There was a problem hiding this comment.
alt attribute should describe the image content. Instead of just 'logo', describe the logo in a way that would make sense to users who cannot see the image.
src/index.html
Outdated
| <nav class="nav"> | ||
| <ul class="nav__list"> | ||
| <li class="nav__item"> | ||
| <a class="nav__link is-active">Apple</a> |
There was a problem hiding this comment.
The href attribute is missing in the <a> tag. Remember to provide a valid reference to make the link functional. Also, avoid using spaces in the href value.
src/index.html
Outdated
| </li> | ||
|
|
||
| <li class="nav__item"> | ||
| <a class="nav__link">Samsung</a> |
There was a problem hiding this comment.
The href attribute is missing in the <a> tag. Remember to provide a valid reference to make the link functional.
src/index.html
Outdated
| </li> | ||
|
|
||
| <li class="nav__item"> | ||
| <a class="nav__link">Smartphones</a> |
There was a problem hiding this comment.
The href attribute is missing in the <a> tag. Remember to provide a valid reference to make the link functional.
src/index.html
Outdated
| </li> | ||
|
|
||
| <li class="nav__item"> | ||
| <a class="nav__link">Photo</a> |
There was a problem hiding this comment.
The href attribute is missing in the <a> tag. Remember to provide a valid reference to make the link functional.
src/index.html
Outdated
| </li> | ||
|
|
||
| <li class="nav__item"> | ||
| <a class="nav__link">Video</a> |
There was a problem hiding this comment.
The href attribute is missing in the <a> tag. Remember to provide a valid reference to make the link functional.
src/style.css
Outdated
| body { | ||
| margin: 0; | ||
| } | ||
|
|
||
| html { | ||
| font-family: Roboto, sans-serif; |
There was a problem hiding this comment.
Consider adding fallback font-families to improve robustness of your CSS in case the primary font isn't available. For example, 'Roboto, Arial, sans-serif'.
| position: relative; | ||
| display: flex; | ||
| height: 60px; | ||
| margin-left: 20px; |
There was a problem hiding this comment.
Be consistent with vertical margins: it's recommended to apply either top or bottom margin, but not both, to elements to avoid unexpected margin collapsing. In this case, you have a margin-left, which is horizontal, but it's good practice to review all margins for consistency.
| .nav__list { | ||
| display: flex; | ||
| list-style: none; | ||
| padding-left: 0; | ||
| margin: 0; |
There was a problem hiding this comment.
Avoid using tag names for styling. You should use classes instead to ensure that the styling is specific and doesn't inadvertently affect other list elements that might be added to the page in the future.
There was a problem hiding this comment.
Hey there! 👋 It looks like there are a few areas that need a bit of tweaking before we can move forward. Specifically, we've got some issues with redundant CSS properties, margin usage that might lead to collapsing margins, missing fallback font-families, incorrect href values, and indentation inconsistencies. These are important for keeping the code clean, maintainable, and functioning as expected. If you could address these points, that would be awesome! Keep up the good work, and I'm looking forward to seeing the improved version! 🚀💻
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| href="images/logo.png" | ||
| > |
There was a problem hiding this comment.
The href attribute in the anchor tag should not point to an image file directly unless it's meant for download. If it's meant to be a link to the homepage, it should have a path like / or #home.
| href="#laptops-computers" | ||
| data-qa="hover" |
There was a problem hiding this comment.
Avoid using spaces in the href attribute of the <a> tag, as anchor links start with the # symbol. The correct value should be #laptops-computers.
| body { | ||
| margin: 0; | ||
| } | ||
|
|
||
| html { | ||
| font-family: Roboto, Arial, sans-serif; |
There was a problem hiding this comment.
Consider specifying fallback font-families in case 'Roboto' is not available. For instance, 'font-family: Roboto, Arial, sans-serif;' ensures that if 'Roboto' fails to load, 'Arial' will be used, and if both custom fonts fail, the browser's default sans-serif font will be used.
| position: relative; | ||
| display: flex; | ||
| height: 60px; | ||
| margin-left: 20px; |
There was a problem hiding this comment.
According to the checklist, be consistent with your vertical margins. It is recommended to use either top or bottom margin, but not both to avoid collapsing margins issues. In this case, 'margin-top' and 'margin-bottom' are set on '.header__link' (lines 60-61), which could potentially lead to such issues.
| justify-content: center; | ||
| } | ||
|
|
||
| .is-active { |
There was a problem hiding this comment.
The '.is-active' class has a 'margin-left' property which is redundant since it's already defined in the '.nav__link' class (line 36) and '.is-active' is expected to override '.nav__link'. If the intention is to set 'margin-left' to 0 for the '.is-active' class, then this is correct. However, if the intention is to maintain the same 'margin-left' as '.nav__link', then this line can be removed.
There was a problem hiding this comment.
🚀 Great job on moving forward! Remember to maintain consistency with your vertical margins, use fallback fonts, fix any incorrect href attributes, and adjust the indentation for better readability. Keep up the good work and address these points in your future code! 🌟
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <a | ||
| class="header__link" | ||
| href="images/logo.png" | ||
| > | ||
| <img | ||
| src="images/logo.png" | ||
| alt="moyo-logo" | ||
| /> |
There was a problem hiding this comment.
The href attribute of the anchor tag pointing to the logo image should likely be the root path or a specific page instead of the image file itself, unless intended for download or a similar function.
| <li class="nav__item"> | ||
| <a | ||
| class="nav__link" | ||
| href="#laptops-computers" |
There was a problem hiding this comment.
The href attribute in the anchor tag should not contain spaces. Replace href="#laptops-computers" with href="#laptops-computers" to correct the URL.
| body { | ||
| margin: 0; | ||
| } | ||
|
|
||
| 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. Consider adding a generic font family as a fallback after 'Arial', such as 'sans-serif'.
| position: relative; | ||
| display: flex; | ||
| height: 60px; | ||
| margin-left: 20px; |
There was a problem hiding this comment.
Be consistent with your vertical margins (Add only top or only bottom margin, but not both). Here you have a left margin, which is fine, but make sure to avoid having both top and bottom margins for vertical consistency.
| margin-top: 10px; | ||
| margin-bottom: 10px; |
There was a problem hiding this comment.
Be consistent with your vertical margins (Add only top or only bottom margin, but not both). Here you've added both top and bottom margins, consider using only one to maintain consistency and to avoid margin collapse issues.
src/style.css
Outdated
| } | ||
|
|
||
| .is-active { | ||
| margin-left: 0; |
There was a problem hiding this comment.
| margin-left: 0; |
it's bad practice to reset the margin in this class, because we can make the third link active, but we don't need in the third link margin-left:0
