Develop #4550
Develop #4550
Conversation
eugenia-andropova
commented
Oct 18, 2024
eugenia-andropova
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Thank you for your hard work on this pull request! There are a few critical issues that need to be addressed before we can proceed. The class naming inconsistencies and the incorrect use of the ::placeholder pseudo-element could lead to functionality issues, and the duplicate 'name' attribute for forms might cause conflicts. Please make these adjustments, and feel free to reach out if you need any help! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/style.css
Outdated
| margin-top: 20px; | ||
| } | ||
|
|
||
| .search-bar::placeholder { |
There was a problem hiding this comment.
The selector .search-bar::placeholder is incorrect. The ::placeholder pseudo-element should be used with an input element, like .search-bar__input::placeholder.
src/style.css
Outdated
| outline: none; | ||
| } | ||
|
|
||
| .search_bar__input--big { |
There was a problem hiding this comment.
There is an inconsistency in class naming. The class .search_bar__input--big uses an underscore instead of a hyphen. It should be .search-bar__input--big to maintain consistency with the BEM naming convention used elsewhere.
src/style.css
Outdated
| background-position: 13px center; | ||
| } | ||
|
|
||
| .search_bar__input--big:hover, |
There was a problem hiding this comment.
There is an inconsistency in class naming. The class .search_bar__input--big:hover uses an underscore instead of a hyphen. It should be .search-bar__input--big:hover to maintain consistency with the BEM naming convention used elsewhere.
src/style.css
Outdated
| box-shadow: 0 3px 8px 0 rgba(61, 78, 97, 0.2); | ||
| } | ||
|
|
||
| .search_bar__input--big:focus, |
There was a problem hiding this comment.
There is an inconsistency in class naming. The class .search_bar__input--big:focus uses an underscore instead of a hyphen. It should be .search-bar__input--big:focus to maintain consistency with the BEM naming convention used elsewhere.
There was a problem hiding this comment.
Great job on your work! 🎉 While there are some areas for improvement, such as adding fallback fonts and refining naming conventions, these are excellent learning opportunities. Remember to address these issues in future projects to enhance your skills. Keep up the great work and continue learning from your experiences!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| <body class="page"> |
There was a problem hiding this comment.
The class name 'page' should represent the meaning of the content rather than the style or tag name. Consider using a more descriptive class name.
| action="#" | ||
| method="post" | ||
| data-qa="big" | ||
| name="search-bar__input--big" |
There was a problem hiding this comment.
The name attribute 'search-bar__input--big' includes double underscores and dashes, which are typically used in class names for BEM (Block Element Modifier) methodology. Consider using a more appropriate naming convention for the name attribute.
| action="#" | ||
| method="post" | ||
| data-qa="small" | ||
| name="search-bar__input--small" |
There was a problem hiding this comment.
The name attribute 'search-bar__input--small' includes double underscores and dashes, which are typically used in class names for BEM (Block Element Modifier) methodology. Consider using a more appropriate naming convention for the name attribute.
| } | ||
|
|
||
| .page { | ||
| font-family: Avenir, sans-serif; |
There was a problem hiding this comment.
Consider adding a fallback font to ensure that if 'Avenir' is not available, the browser can use an alternative font. This aligns with the checklist item: 'Remember to use fallback fonts - alternative font-family in case the main one doesn't work'.
