Develop #4468
Develop #4468
Conversation
AndriiKasiian
commented
Sep 15, 2024
•
AndriiKasiian
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Great job on structuring your CSS! While there are a few points to consider for improvement, none of them are critical issues that would warrant a REQUEST_CHANGES decision. Keep up the good work and consider the feedback to further refine your code in the future. Remember, attention to detail can make your code even more maintainable and scalable! 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <form | ||
| class="search-container search-container_big" | ||
| data-qa="big" | ||
| > | ||
| <label class="search__label search__label_big"> | ||
| <input | ||
| class="search-bar search-bar_big" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| </label> |
There was a problem hiding this comment.
There should be an empty line between sibling form elements to add some 'air' and simplify reading. Please add an empty line between the closing </form> tag on line 32 and the opening <form> tag on line 33.
| <body class="page"> | ||
| <form | ||
| class="search-container search-container_big" | ||
| data-qa="big" | ||
| > | ||
| <label class="search__label search__label_big"> | ||
| <input | ||
| class="search-bar search-bar_big" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| </label> | ||
| </form> | ||
| <form | ||
| class="search-container search-container_small" | ||
| data-qa="small" | ||
| > | ||
| <label class="search__label search__label_small"> | ||
| <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.
Remember about correct indentation between parent and child elements. The form elements and their children should have a 2-space offset from their parent body tag.
| * { | ||
| padding: 0; | ||
| box-sizing: border-box; | ||
| } |
There was a problem hiding this comment.
Don't use * selector (it impacts performance). Set styles only for elements that require them. Zeroing out your margins, paddings or other styles with '*' is still inefficient for the browser.
| } | ||
|
|
||
| body { | ||
| font-family: Avenir, 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. It's good practice to specify a generic font family as a fallback. For example, 'font-family: Avenir, Arial, sans-serif;' ensures that if Avenir is not available, Arial will be used before falling back to the default sans-serif font.
| border-radius: 4px; | ||
| color: #3d4e61; | ||
| font-weight: 300; | ||
| font-family: inherit; |
There was a problem hiding this comment.
It's recommended to have a fallback font-family in case the inherited font is not available. Consider adding a generic font family as a backup.
|
|
||
| .search-container_big { | ||
| width: 100%; | ||
| height: 70px; |
There was a problem hiding this comment.
Don't fix container size (if there is no such a requirement). The fixed height can lead to content overflow or accidental scroll bars if the content exceeds the container's height. Consider using padding and allow the content to define the height.
|
|
||
| .search-container_small { | ||
| width: 100%; | ||
| height: 42px; |
There was a problem hiding this comment.
Don't fix container size (if there is no such a requirement). As with the previous comment, avoid using a fixed height unless it's a specific requirement. Content should dictate the height of the container to prevent overflow issues.
| position: relative; | ||
| display: block; | ||
| width: 100%; | ||
| height: 100%; |
There was a problem hiding this comment.
Avoid fixing the container size with 100% height unless there is a specific requirement. It's better to let the content size dictate it to avoid overflow or accidental scroll bars.
| width: 19px; | ||
| height: 19px; | ||
| top: 25px; | ||
| left: 26px; |
There was a problem hiding this comment.
Consider using a CSS variable for the background image URL since it's repeated in both .search__label_big::after and .search__label_small::after. This will make it easier to manage if the image needs to be changed in the future.
| top: 25px; | ||
| left: 26px; |
There was a problem hiding this comment.
The fixed pixel values for top and left could potentially cause issues if the label size changes. Consider using relative units like percentages or em for better scalability.
| - [DEMO LINK](https://AndriiKasiian.github.io/layout_search-bar-airbnb/) | ||
| - [TEST REPORT LINK](https://AndriiKasiian.github.io/layout_search-bar-airbnb/report/html_report/) |
There was a problem hiding this comment.
Add these links to the PR description
