add search-bar #4466
add search-bar #4466
Conversation
SerhiyPaziyc
commented
Sep 11, 2024
SerhiyPaziyc
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
You've put in some great effort here! 🌟 However, before we can move forward, we need to address a few key points to ensure your code is both functional and follows best practices. Please take a moment to correct the issues with the src attribute in input elements, and refine your class naming to accurately represent your content. Additionally, let's make sure the alt text for images is descriptive of their function. These changes will greatly enhance the accessibility and maintainability of your project. Keep up the good work, and with these tweaks, 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
| class="search" | ||
| data-qa="big" | ||
| > | ||
| <button class="search--button"> |
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--button' seems to indicate a BEM modifier, but it is not clear what it is modifying.
src/index.html
Outdated
| <img | ||
| class="search_img search--img--big" | ||
| src="/src/images/Search.svg" | ||
| alt="button" | ||
| /> |
There was a problem hiding this comment.
The alt attribute should describe the image content, not just 'button'. It should convey the function or purpose of the image, for example 'Search icon'.
src/index.html
Outdated
| > | ||
| <button class="search--button"> | ||
| <img | ||
| class="search_img search--img--big" |
There was a problem hiding this comment.
Class names should represent the meaning of the content, not the styles or tag names. Avoid using multiple modifiers in BEM and ensure that the class name is meaningful.
src/index.html
Outdated
| class="search" | ||
| data-qa="small" | ||
| > | ||
| <button class="search--button"> |
There was a problem hiding this comment.
Ensure that class names represent the content meaningfully. The class 'search--button' does not clearly reflect its purpose or content.
src/index.html
Outdated
| <img | ||
| class="search_img search--img--small" | ||
| src="/src/images/Search.svg" | ||
| alt="button" | ||
| /> |
There was a problem hiding this comment.
The alt attribute for the image should describe the content, such as 'Search icon', instead of just 'button'.
src/index.html
Outdated
| > | ||
| <button class="search--button"> | ||
| <img | ||
| class="search_img search--img--small" |
There was a problem hiding this comment.
Class names should be meaningful and represent the content. The class 'search_img search--img--small' seems to mix BEM with other naming conventions, which can be confusing.
src/index.html
Outdated
| type="text" | ||
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| src="/src/images/Search.svg" |
There was a problem hiding this comment.
The src attribute should not be used with input elements. If you meant to link an image for a background, consider using CSS instead.
src/style.css
Outdated
|
|
||
| .search:hover, | ||
| .search:focus-within { | ||
| box-shadow: 0 4px 4px rgba(0, 0, 0, 0.25); |
There was a problem hiding this comment.
While using rgba for the box-shadow color is fine, it's a good practice to be consistent with color formats throughout your CSS. You've used hex color codes for borders and shadows elsewhere, so consider using the rgba equivalent of #3d4e611a for consistency.
src/style.css
Outdated
|
|
||
| .search--big::placeholder, | ||
| .search--small::placeholder { | ||
| font-family: Avenir, serif; |
There was a problem hiding this comment.
The font-family for placeholders should include a fallback font. In this case, you have specified 'serif' as a fallback for Avenir, but it would be more consistent with the body font stack to use 'Arial, Helvetica, sans-serif' as the fallback.
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| margin-top: 20px; |
There was a problem hiding this comment.
Be consistent with your margins. It's recommended to add only top or bottom margins, but not both, to avoid potential margin collapse issues. In this case, you have only a top margin, which is fine, but ensure that the rest of your elements follow this practice as well.
