-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add solution #4558
base: master
Are you sure you want to change the base?
add solution #4558
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there are a few critical issues that need to be addressed before we can approve this Pull Request. The use of 'img' tags instead of the recommended 'background-image' CSS property for icons, and the incorrect placement of the search bar outside of a 'form' tag, are important to fix to meet the core requirements. 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! ✨
🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review
button in the Reviewers tab.
src/index.html
Outdated
<div | ||
class="search-bar" | ||
data-qa="big" | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'div' element for the search bar should be inside a 'form' tag as per the checklist requirement: 'Inputs are written inside of 'form' tag with correctly passed attributes'.
src/index.html
Outdated
<div | ||
class="search-bar" | ||
data-qa="small" | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 'div' element for the search bar should be inside a 'form' tag as per the checklist requirement: 'Inputs are written inside of 'form' tag with correctly passed attributes'.
src/index.html
Outdated
<img | ||
class="image-big" | ||
src="images/Search.svg" | ||
alt="search-big" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checklist specifies that the icon should be implemented using the background-image CSS property instead of an 'img' tag.
src/index.html
Outdated
<img | ||
class="image-small" | ||
src="images/Search.svg" | ||
alt="search-small" | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checklist specifies that the icon should be implemented using the background-image CSS property instead of an 'img' tag.
src/style.css
Outdated
img { | ||
overflow-clip-margin: content-box; | ||
overflow: clip; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using tag names for styling except for 'html' and 'body'. Instead, use classes or other selectors as per the checklist: 'Don't use tag names for styling (except html and body)'.
src/style.css
Outdated
.image-big { | ||
width: 19px; | ||
height: 19px; | ||
padding-left: 6px; | ||
padding-right: 17px; | ||
top: 25px; | ||
left: 26px; | ||
} | ||
|
||
.image-small { | ||
width: 11px; | ||
height: 11px; | ||
padding-bottom: 1px; | ||
padding-right: 8px; | ||
top: 15px; | ||
left: 13px; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checklist suggests using the background-image CSS property for icons instead of using 'img' tags. Consider applying this to the '.image-big' and '.image-small' classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on your submission! 🎉 While there are a few issues to address, such as correcting the CSS selectors for focus and hover states, removing the color
property from the @font-face
rule, combining src
properties, and properly closing the form tag, these are excellent learning opportunities. Remember to review the comments to enhance your code. Addressing these critical issues will help you avoid similar problems in the future. Keep up the good work and continue improving! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
<form | ||
class="search-bar-top" | ||
data-qa="big" | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The form tag should be closed properly. Currently, it is self-closing which is incorrect for a form element. Ensure the form tag is opened and closed correctly.
<form | ||
class="search-bar-down" | ||
data-qa="small" | ||
> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The form tag should be closed properly. Currently, it is self-closing which is incorrect for a form element. Ensure the form tag is opened and closed correctly.
src: url('./fonts/Avenir-Book.ttf'); | ||
src: url('./fonts/Avenir-Heavy.ttf'); | ||
src: url('./fonts/Avenir.ttc'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having multiple src
properties in the @font-face
rule is not correct. You should combine them into a single src
property using a comma to separate different font files. This ensures that the browser can choose the best format it supports.
src: url('./fonts/Avenir.ttc'); | ||
font-style: normal; | ||
font-weight: normal; | ||
color: rgba(61, 78, 97, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The color
property is not applicable within the @font-face
rule. You should remove it as it doesn't affect the font face definition.
color: #3d4e61; | ||
} | ||
|
||
.search-bar-input :hover { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The selector .search-bar-input :hover
is incorrect. There should not be a space between .search-bar-input
and :hover
. It should be .search-bar-input:hover
to apply the hover effect correctly to the input element.
color: rgba(0, 0, 0, 0.25); | ||
} | ||
|
||
.search-bar-input :focus { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The selector .search-bar-input :focus
is incorrect. There should not be a space between .search-bar-input
and :focus
. It should be .search-bar-input:focus
to apply the focus effect correctly to the input element.
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 Mistakes
fromBEM
lesson theory are checked.Code follows all the Code Style Rules ❗️