task solution #4473
task solution #4473
Conversation
D0dgerJ
commented
Sep 18, 2024
D0dgerJ
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Great job on the Pull Request! There are a few minor issues to keep in mind for the future, such as ensuring browser compatibility with properties like 'margin-block' and adding a generic font family as a fallback. Also, remember to use actual Unicode characters for quotes and maintain consistent indentation for better readability. Keep up the good work! 😊👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <form | ||
| class="page__form" | ||
| action="#" | ||
| data-qa="big" |
There was a problem hiding this comment.
Keep your attributes correctly formatted. If the HTML element has long attribute values or the number of attributes is more than 2 - start each one, including the first, on the new line with 2-space indentation related to the tag.
| class="search-bar search-bar--big" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try "Los Angeles"" |
There was a problem hiding this comment.
Avoid using HTML character references for quotes in attribute values if possible. Instead, use the actual Unicode characters or escape sequences. This improves readability.
| class="search-bar search-bar--small" | ||
| type="text" | ||
| data-qa="hover" | ||
| placeholder="Try "Los Angeles"" |
There was a problem hiding this comment.
Avoid using HTML character references for quotes in attribute values if possible. Instead, use the actual Unicode characters or escape sequences. This improves readability.
|
|
||
| html { | ||
| color: #3d4e61; | ||
| font-family: Avenir, Helvetica, sans-serif; |
There was a problem hiding this comment.
It's great that you've included a fallback font for Avenir. However, remember to include a generic family as well, such as 'serif' or 'sans-serif', to cover all bases in case the specified fonts are unavailable.
| } | ||
|
|
||
| .page__form { | ||
| margin-top: 20px; |
There was a problem hiding this comment.
Be consistent with your margins. You've added a top margin here, so ensure you're not adding bottom margins to other elements that are vertically adjacent to avoid potential margin collapse issues.
| } | ||
|
|
||
| body { | ||
| margin-block: 0; |
There was a problem hiding this comment.
Using 'margin-block' is not a common practice and may not be supported in all browsers. It's better to stick with 'margin-top' and 'margin-bottom' for clarity and broader compatibility.
| line-height: 19px; | ||
| background-position: 13px center; | ||
| background-size: 11px; | ||
| font-weight: 400; |
There was a problem hiding this comment.
You've set the font-weight to 400 here, which is unnecessary if it's the same as the inherited weight. If it's meant to override a different weight, then it's fine to keep it.
