Solution #4499
Solution #4499
Conversation
There was a problem hiding this comment.
Hey there! 👋 It looks like there are a few areas that need a bit of tweaking to align with best practices and maintain consistency in your code. Remember, keeping your code well-organized and consistent makes it easier to read and maintain for you and others in the future. Keep up the great work, and with some minor adjustments, your code will be top-notch! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| padding: 0; | ||
| } | ||
|
|
||
| .search-bar { |
There was a problem hiding this comment.
Consider adding a bottom margin to the .search-bar instead of a top margin to be consistent with your margins, as per the checklist item 'Be consistent with your margins (Add only top or bottom, but not both)'.
| text-shadow: 0 4px 4px rgba(0, 0, 0, 0.25); | ||
| } | ||
|
|
||
| .search-bar__input--small { |
There was a problem hiding this comment.
The .search-bar__input--small class correctly overrides the necessary properties for the smaller search bar variant, maintaining consistency in the design. However, make sure that the font-weight: 400; is necessary here, as the normal font weight is usually the default and might not need to be specified unless it's overriding a previous setting.
There was a problem hiding this comment.
Great job on your HTML structure! 👍 It's mostly well-constructed with proper semantic elements and attributes. Just a few minor formatting issues to iron out for the future. Keep up the good work and always aim for clean, maintainable code! Remember, attention to detail can make a big difference! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| <input | ||
| class="search-bar__input" | ||
| data-qa="keypress" | ||
| type="text" | ||
| placeholder="Try “Los Angeles”" | ||
| /> |
There was a problem hiding this comment.
Keep your attributes correctly formatted. Since the input element has more than two attributes, each attribute should start on a new line with 2-space indentation relative to the tag.
| <input | ||
| class="search-bar__input search-bar__input--small" | ||
| type="text" | ||
| placeholder="Try “Los Angeles”" |
There was a problem hiding this comment.
Keep your attributes correctly formatted. Since the input element has more than two attributes, each attribute should start on a new line with 2-space indentation relative to the tag.
| @font-face { | ||
| font-family: Avenir; | ||
| src: url('fonts/Avenir.ttc'); | ||
| font-weight: 300; | ||
| } | ||
|
|
||
| @font-face { | ||
| font-family: Avenir; | ||
| src: url('fonts/Avenir.ttc'); | ||
| font-weight: bold; |
There was a problem hiding this comment.
Consider adding a fallback font family in your @font-face rule to ensure text remains visible when the custom font is loading or if it fails to load. For example, you could modify the font-family property to include a generic font family like sans-serif as a backup.
| } | ||
|
|
||
| body { | ||
| padding: 0; |
There was a problem hiding this comment.
It's a good practice to set margin: 0; along with padding: 0; on the body element to reset any default margins provided by the browser.
| } | ||
|
|
||
| .search-bar { | ||
| margin-top: 20px; |
There was a problem hiding this comment.
Be consistent with your margins. Since you're adding a margin to the top, ensure that you're not adding margin to the bottom of any elements within .search-bar to avoid potential margin collapse issues.
| background-position: 25px; | ||
| width: 100%; | ||
| height: var(--search-bar-height); | ||
| padding-left: 62px; |
There was a problem hiding this comment.
Avoid fixing the padding size. Instead, consider using relative units or a variable if the design allows for flexibility. This can make the design more responsive and easier to maintain.
Removed sans-serif from Font-face
There was a problem hiding this comment.
Great job on your Pull Request! 🌟 There are some minor issues related to code style and best practices, like consistent margins, avoiding tag selectors, using semantic tags, and proper attribute formatting. However, these don't significantly impact the functionality or security of your code. Keep these tips in mind for future projects 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-bar" | ||
| data-qa="big" | ||
| > | ||
| <input | ||
| class="search-bar__input" | ||
| data-qa="keypress" | ||
| type="text" | ||
| placeholder="Try “Los Angeles”" | ||
| /> |
There was a problem hiding this comment.
Keep your attributes correctly formatted. Since the <input> element on lines 25-30 has more than two attributes, each attribute should start on a new line with 2-space indentation relative to the tag.
| <form | ||
| class="search-bar" | ||
| data-qa="small" | ||
| > | ||
| <input | ||
| class="search-bar__input search-bar__input--small" | ||
| type="text" | ||
| placeholder="Try “Los Angeles”" | ||
| /> |
There was a problem hiding this comment.
Keep your attributes correctly formatted. Since the <input> element on lines 36-40 has more than two attributes, each attribute should start on a new line with 2-space indentation relative to the tag.
| class="search-bar__input" | ||
| data-qa="keypress" | ||
| type="text" | ||
| placeholder="Try “Los Angeles”" |
There was a problem hiding this comment.
Use semantic tags where possible. Consider wrapping the <input> elements in a semantic tag like <section> or <div> to indicate a block of related content.
| body { | ||
| padding: 0; | ||
| } |
There was a problem hiding this comment.
Don't use tag names for styling (except html and body). Instead of setting padding directly on the body, use a class to style these properties.
| .search-bar { | ||
| 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.
| background-repeat: no-repeat; | ||
| background-size: var(--search-icon-size); | ||
| background-position: 25px; | ||
| width: 100%; |
There was a problem hiding this comment.
Don't fix container size (if there is no such a requirement). The width of the .search-bar__input is set to 100%, which is good as it allows the container to be flexible. However, ensure that this is not causing any overflow or accidental scrollbar, especially when padding or borders are added.
DEMO LINK
TEST REPORT LINK
Icon implemented using background-image CSS property
Inputs are written inside of 'form' tag with correctly passed attributes
All
Typical MistakesfromBEMlesson theory are checked.Code follows all the Code Style Rules ❗️