Search bar task solution #4477
Search bar task solution #4477
Conversation
Nastiko-ko
commented
Sep 20, 2024
Nastiko-ko
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Hey there! It looks like there are a few important issues to address in your code before we can give it the green light. 🚦 Specifically, you'll want to make sure your font-family declarations have fallbacks, and your @font-face declarations are separated by font weight. Also, pay attention to the formatting of your attributes and indentation, as well as the spacing between blocks of HTML. These might seem like small details, but they're super important for keeping your code clean and maintainable! Once you tidy up these areas, you'll be all set! 🛠️
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <body> | ||
| <input | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| <form | ||
| class="search-form" | ||
| action="#" | ||
| data-qa="big" | ||
| > |
There was a problem hiding this comment.
Remember to add empty lines between multiline sibling blocks of HTML. In this case, there should be an empty line between the closing tag on line 31 and the opening
tag on line 33.| <input | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| class="search-input search-input--big" |
There was a problem hiding this comment.
Keep your attributes correctly formatted. For the element starting on line 25, each attribute should start on a new line with a 2-space indentation from the tag.
| <input | ||
| type="text" | ||
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| class="search-input search-input--small" |
There was a problem hiding this comment.
The attributes of the element starting on line 38 should also be formatted correctly, with each attribute starting on a new line with a 2-space indentation from the tag.
src/style.css
Outdated
| /* add styles here */ | ||
| @font-face { | ||
| font-family: Avenir; | ||
| src: url(./fonts/Avenir-Book.ttf), url(./fonts/Avenir-Heavy.ttf); |
There was a problem hiding this comment.
It's more efficient to define multiple font weights in separate @font-face declarations, each with its own font-weight property, rather than combining them in a single src property.
src/style.css
Outdated
| border: 1px solid #e1e7ed; | ||
| border-radius: 4px; | ||
| box-shadow: 0 1px 8px 0 #3d4e611a; | ||
| font-family: Avenir, sans-serif; |
There was a problem hiding this comment.
You should include fallback fonts in the font-family property to ensure text is displayed with a default font in case the custom font fails to load. For example: font-family: Avenir, Arial, sans-serif;.
| .search-form { | ||
| margin-top: 20px; | ||
| } |
There was a problem hiding this comment.
According to the checklist, it's recommended to use either top or bottom margin for consistency and to avoid potential margin collapse issues. Consider using only margin-bottom for elements and removing margin-top.
There was a problem hiding this comment.
Great job on the Pull Request! 🌟 You've done well addressing the main functionality and security aspects. Just a few minor formatting issues were noted, such as font properties and indentation. It's always good to keep your code clean and consistent, but these small tweaks don't hinder the approval. Keep up the good work, and consider these tips for your next project to make your code even better! 💻✨
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <form | ||
| class="search-form" | ||
| action="#" | ||
| data-qa="big" | ||
| > |
There was a problem hiding this comment.
The attributes in the
tag should start on a new line with a 2-space indentation relative to the tag for better readability as per the checklist.| <input | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| class="search-input search-input--big" |
There was a problem hiding this comment.
The attributes in the tag should also follow the same formatting rule as the
tag, with each attribute on a new line and properly indented.| <form | ||
| class="search-form" | ||
| action="#" | ||
| data-qa="small" | ||
| > |
There was a problem hiding this comment.
As with the previous
tag, please format the attributes correctly with each on a new line and a 2-space indentation.| <input | ||
| type="text" | ||
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| class="search-input search-input--small" |
There was a problem hiding this comment.
The attributes in this tag should also be formatted correctly, with each attribute on a new line and a 2-space indentation.
src/style.css
Outdated
| font-weight: 300; | ||
| font-weight: bold; |
There was a problem hiding this comment.
There is a duplicate font-weight property in this @font-face rule. Only the last one (bold) will be used, and the previous one (300) will be ignored. You should remove the first font-weight declaration to avoid confusion and clean up the code.
| border: 1px solid #e1e7ed; | ||
| border-radius: 4px; | ||
| box-shadow: 0 1px 8px 0 #3d4e611a; | ||
| font-family: Avenir, 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. For instance, you could specify a generic font family like sans-serif as a backup.
|
|
||
| .search-input:focus { | ||
| box-shadow: 0 4px 4px 0 #00000040; | ||
| font-weight: 900; |
There was a problem hiding this comment.
The font-weight property here is set to 900, which might not be supported by the Avenir font as per the defined @font-face rules. Ensure there's a corresponding @font-face rule that supports the 900 font-weight or adjust this value accordingly.
