add task solution #4484
add task solution #4484
Conversation
miroshnykow
commented
Sep 24, 2024
miroshnykow
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Hey there! 👋 It looks like there are a few things to address in your code. The image paths should be consistent, and margins should be standardized to prevent layout issues. Also, don't forget to include a fallback font and to keep attributes formatted correctly. Lastly, the class names should be meaningful and the indentation consistent. Keep up the good work, and with these adjustments, your code will be even better! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| <body> | ||
| <input | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| <form | ||
| action="#" | ||
| data-qa="big" | ||
| > | ||
| <label> | ||
| <input | ||
| class="search-bar search-bar__big" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| </label> | ||
| </form> | ||
|
|
||
| <input | ||
| type="text" | ||
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| <form | ||
| action="#" | ||
| data-qa="small" | ||
| > | ||
| <label> | ||
| <input | ||
| class="search-bar search-bar__small" | ||
| type="text" | ||
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| </label> | ||
| </form> |
There was a problem hiding this comment.
There should be empty lines between sibling form blocks to simplify reading and add some "air" to the code structure.
src/index.html
Outdated
| data-qa="big" | ||
| > | ||
| <label> | ||
| <input |
There was a problem hiding this comment.
Consider using a div or section element to wrap the input field inside the label for better semantics and style targeting.
src/index.html
Outdated
| <label> | ||
| <input | ||
| class="search-bar search-bar__big" | ||
| type="text" |
There was a problem hiding this comment.
Class names should represent the meaning of the content, not the styles or tag names. The class name 'search-bar__big' suggests a BEM naming convention but is being used to indicate the size. Consider a more meaningful name that describes the purpose or context instead of the size.
src/index.html
Outdated
| > | ||
| <label> | ||
| <input | ||
| class="search-bar search-bar__small" |
There was a problem hiding this comment.
Similar to the previous comment, 'search-bar__small' should be replaced with a class name that reflects the function or context of the element rather than its size.
src/index.html
Outdated
| > | ||
| <label> | ||
| <input | ||
| class="search-bar search-bar__big" |
There was a problem hiding this comment.
The type attribute value 'text' should be on the same line as the class attribute, following the rule that if an element has more than two attributes, each one, including the first, should start on a new line with 2-space indentation.
src/index.html
Outdated
| data-qa="small" | ||
| > | ||
| <label> | ||
| <input |
There was a problem hiding this comment.
The type attribute value 'text' should be on the same line as the class attribute for consistency with the formatting rule for multiple attributes.
| } | ||
|
|
||
| .search-bar { | ||
| font-family: Avenir, sans-serif; |
There was a problem hiding this comment.
Consider adding a fallback font family after 'Avenir'. If 'Avenir' fails to load for some reason, the browser will use the fallback font instead.
| box-sizing: border-box; | ||
| width: 100%; | ||
|
|
||
| margin-top: 20px; |
There was a problem hiding this comment.
According to the checklist, be consistent with your margins. Choose either top or bottom margins for elements, but not both, to avoid potential margin collapse issues.
| .search-bar__small:focus { | ||
| border: 1px solid #e1e7ed; | ||
| text-shadow: none; | ||
| background-image: url(/src/images/Search.svg), |
There was a problem hiding this comment.
Make sure the path to the image is consistent across the CSS file. Here, the path starts with '/src/images/Search.svg', while elsewhere it is 'images/Search.svg'. Use a consistent path to ensure the image loads correctly in all cases.
There was a problem hiding this comment.
Great job on your Pull Request! There are a few minor suggestions like considering responsive design for margins, using fallback font-families, and improving class names for better clarity. Also, keep an eye on the formatting of your attributes and indentation for a cleaner code structure. These are not critical issues, so your PR is approved! Keep up the good work, and remember that these small improvements can make your code even better in the future! 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <form | ||
| action="#" | ||
| data-qa="big" | ||
| > |
There was a problem hiding this comment.
Keep your attributes correctly formatted. If the element has more than 2 attributes, start each one on a new line with a 2-space indentation related to the tag.
src/index.html
Outdated
| <input | ||
| class="search-bar search-bar__big" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" |
There was a problem hiding this comment.
The input element's attributes should be formatted according to the guidelines. Each attribute should start on a new line with a 2-space indentation.
| <form | ||
| action="#" | ||
| data-qa="small" | ||
| > |
There was a problem hiding this comment.
Keep your attributes correctly formatted. If the element has more than 2 attributes, start each one on a new line with a 2-space indentation related to the tag.
src/index.html
Outdated
| <input | ||
| class="search-bar search-bar__small" | ||
| type="text" | ||
| data-qa="hover" | ||
| placeholder="Los Angeles" |
There was a problem hiding this comment.
The input element's attributes should be formatted according to the guidelines. Each attribute should start on a new line with a 2-space indentation.
src/index.html
Outdated
| <input | ||
| class="search-bar search-bar__big" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" |
There was a problem hiding this comment.
Class names should represent the meaning of the content, not the styles or tag names. The class search-bar__big could be misinterpreted as a style rather than the meaning of the content.
src/index.html
Outdated
| <input | ||
| class="search-bar search-bar__small" | ||
| type="text" | ||
| data-qa="hover" | ||
| placeholder="Los Angeles" |
There was a problem hiding this comment.
Class names should represent the meaning of the content, not the styles or tag names. The class search-bar__small could be misinterpreted as a style rather than the meaning of the content.
| } | ||
|
|
||
| .search-bar { | ||
| font-family: Avenir, sans-serif; |
There was a problem hiding this comment.
Consider adding a fallback font-family for Avenir in case it fails to load. For example, 'font-family: Avenir, Arial, sans-serif;'. This ensures text is still readable with a default font.
| box-sizing: border-box; | ||
| width: 100%; | ||
|
|
||
| margin-top: 20px; |
There was a problem hiding this comment.
According to the checklist, be consistent with your margins. It's recommended to add margins only to the top or bottom of elements, not both, to avoid potential margin collapse.
| box-sizing: border-box; | ||
| width: 100%; | ||
|
|
||
| margin-top: 20px; |
There was a problem hiding this comment.
The .search-bar class has a fixed margin-top, which might not be flexible for different contexts. Consider using a more responsive design approach unless a fixed size is a specific requirement.
|
|
||
| <label> | ||
|
|
||
| <div> |
There was a problem hiding this comment.
Redundant
| <div> |
|
|
||
| <label> | ||
|
|
||
| <div> |
