add task solution #4479
add task solution #4479
Conversation
There was a problem hiding this comment.
While you're doing great, we need to address a few important issues before we can merge your Pull Request. There are concerns about fixed container sizes, which could lead to content overflow, the use of the universal selector '*', and potential margin collapse due to setting both top and bottom margins. Please take a moment to review and adjust these areas. Remember, your hard work is truly appreciated, and these changes will ensure your code is top-notch! Keep up the good work! 🌟
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| <div class="search-bar_container"> |
There was a problem hiding this comment.
Consider using a semantic tag such as <header> or <section> for the search bar container instead of a non-semantic <div>, to improve the structure and SEO of your page.
src/index.html
Outdated
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| <div class="search-bar_container"> |
There was a problem hiding this comment.
The class name 'search-bar_container' should represent the meaning of the content, not the styles or tag names. A more semantic name like 'search-container' might be better.
| class="search-bar top-search-bar" | ||
| data-qa="big" | ||
| > | ||
| <input |
There was a problem hiding this comment.
The alt attribute is missing on the input element. Although it's not required for input elements, if you had images, remember to provide meaningful alt text for images.
| * { | ||
| font-family: Avenir-Book, Arial, Helvetica, sans-serif; | ||
| font-weight: 300; | ||
| font-style: normal; | ||
| color: rgba(61, 78, 97, 1); | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
The '' selector impacts performance and should not be used. It's better to set styles specifically for elements that require them. Also, avoid setting global font styles on the '' selector; use 'body' or specific classes instead.
| } | ||
|
|
||
| body { | ||
| margin-top: 0; |
There was a problem hiding this comment.
According to the checklist, be consistent with your margins by adding only top or bottom, but not both. It seems you have set the top margin here, ensure that you're not also setting the bottom margin elsewhere for the same element to avoid potential margin collapse.
| } | ||
|
|
||
| .search-bar { | ||
| margin-top: 20px; |
There was a problem hiding this comment.
Following the same principle as the previous comment, ensure that you're not setting both top and bottom margins for elements to avoid potential margin collapse issues.
| background-position: top 15px left 13px; | ||
| background-size: 11px 11px; | ||
| font-size: 14px; | ||
| } |
There was a problem hiding this comment.
It is recommended not to fix container sizes unless there's a specific requirement for it. Ensure that the fixed heights for the '.search-bar-input--top' and '.search-bar-input--bottom' are indeed necessary and that the content does not overflow or create an accidental scrollbar.
There was a problem hiding this comment.
Great job on your Pull Request! 🌟 All the feedback provided is about minor issues related to code style and best practices. None of them are critical enough to warrant a REQUEST_CHANGES decision. Keep up the good work, and consider these tips to enhance your code in the future! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <header class="search-container"> | ||
| <form | ||
| class="search-bar top-search-bar" | ||
| data-qa="big" | ||
| > | ||
| <input | ||
| class="search-bar-input search-bar-input--top" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| </form> |
There was a problem hiding this comment.
Consider adding an empty line between the header tag and the form tag to add some 'air' and simplify reading, as they are sibling elements. However, do not add an empty line between the form and its child input as they are parent and child elements.
| <form | ||
| class="search-bar top-search-bar" | ||
| data-qa="big" | ||
| > | ||
| <input | ||
| class="search-bar-input search-bar-input--top" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" |
There was a problem hiding this comment.
The attributes in the input tag should start on a new line with 2-space indentation relative to the tag, and the closing bracket should be on the same level as the opening one. Please adjust the formatting of the input tag to match the guideline.
| <form | ||
| class="search-bar bottom-search-bar" | ||
| data-qa="small" | ||
| > | ||
| <input | ||
| class="search-bar-input search-bar-input--bottom" | ||
| type="text" | ||
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| /> |
There was a problem hiding this comment.
The attributes in the input tag should start on a new line with 2-space indentation relative to the tag, and the closing bracket should be on the same level as the opening one. Please adjust the formatting of the input tag to match the guideline.
| * { | ||
| font-family: Avenir-Book, Arial, Helvetica, sans-serif; | ||
| font-weight: 300; | ||
| font-style: normal; | ||
| color: rgba(61, 78, 97, 1); | ||
| box-sizing: border-box; |
There was a problem hiding this comment.
Avoid using the * selector as it impacts performance. It's better to set styles only for elements that require them.
| } | ||
|
|
||
| * { | ||
| font-family: Avenir-Book, Arial, Helvetica, sans-serif; |
There was a problem hiding this comment.
Remember to use fallback fonts. It's good you have Arial and Helvetica, but you should also include a generic family like 'sans-serif' at the end.
| body { | ||
| margin-top: 0; | ||
| } |
There was a problem hiding this comment.
Be consistent with your margins. Add only top or bottom margins, but not both, to avoid potential margin collapse issues.
| .search-bar { | ||
| margin-top: 20px; | ||
| } |
There was a problem hiding this comment.
You have a margin-top on the .search-bar class. If there are no specific requirements for the container size, consider letting the content size dictate the spacing instead of fixing it with margins.
| .search-bar-input { | ||
| width: 100%; | ||
| height: 100%; | ||
| background-image: url(images/Search.svg); | ||
| background-repeat: no-repeat; | ||
| border-radius: 4px; | ||
| border: 1px solid #e1e7ed; | ||
| box-shadow: 0 1px 8px 0 #3d4e611a; | ||
| box-sizing: border-box; | ||
| } |
There was a problem hiding this comment.
Avoid fixing container sizes like width and height unless required. Let the content size dictate the container size to avoid overflow or accidental scroll bars.
Typical MistakesfromBEMlesson theory are checked.