-
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
initial task solution #4563
base: master
Are you sure you want to change the base?
initial task solution #4563
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.
Thank you for your hard work on this! 😊 There are a few critical issues that need to be addressed before we can approve the pull request. The missing units for 'background-size' and 'background-position' properties could affect the rendering of your design, and it's important to ensure the 'font-weight' matches the available font files to maintain consistency. Please make these adjustments, and feel free to reach out if you need any help! Keep up the great work! 🚀
✨ 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/blocks/big-search.css
Outdated
border: none; | ||
background-image: url(../images/Search.svg); | ||
background-repeat: no-repeat; | ||
background-size: 19; |
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 'background-size' property is missing a unit. It should be specified in a valid CSS unit like 'px', 'em', etc.
src/blocks/big-search.css
Outdated
background-image: url(../images/Search.svg); | ||
background-repeat: no-repeat; | ||
background-size: 19; | ||
background-position: 26 24; |
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 'background-position' property is missing units. Both values should be specified in a valid CSS unit like 'px', 'em', etc.
src/blocks/big-search.css
Outdated
background: linear-gradient(180deg, #fff 0%, #f6f6f7 100%); | ||
background-image: url(../images/Search.svg); | ||
background-repeat: no-repeat; | ||
background-size: 19; |
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 'background-size' property is missing a unit. It should be specified in a valid CSS unit like 'px', 'em', etc.
src/blocks/big-search.css
Outdated
background-image: url(../images/Search.svg); | ||
background-repeat: no-repeat; | ||
background-size: 19; | ||
background-position: 26 24; |
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 'background-position' property is missing units. Both values should be specified in a valid CSS unit like 'px', 'em', etc.
border: none; | ||
background-image: url(../images/Search.svg); | ||
background-repeat: no-repeat; | ||
background-size: 11; |
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 'background-size' property is missing a unit. It should be specified in a valid CSS unit like 'px', 'em', etc.
src/blocks/small-search.css
Outdated
box-shadow: 0 3px 8px 0 #3d4e6133; | ||
background-image: url(../images/Search.svg); | ||
background-repeat: no-repeat; | ||
background-size: 11; |
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 'background-size' property is missing a unit. It should be specified in a valid CSS unit like 'px', 'em', etc.
src/blocks/small-search.css
Outdated
background-image: url(../images/Search.svg); | ||
background-repeat: no-repeat; | ||
background-size: 11; | ||
background-position: 13 14; |
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 'background-position' property is missing units. Both values should be specified in a valid CSS unit like 'px', 'em', etc.
src/blocks/small-search.css
Outdated
border-radius: 3px; | ||
background-image: url(../images/Search.svg); | ||
background-repeat: no-repeat; | ||
background-size: 11; |
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 'background-size' property is missing a unit. It should be specified in a valid CSS unit like 'px', 'em', etc.
src/blocks/small-search.css
Outdated
background-image: url(../images/Search.svg); | ||
background-repeat: no-repeat; | ||
background-size: 11; | ||
background-position: 13 14; |
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 'background-position' property is missing units. Both values should be specified in a valid CSS unit like 'px', 'em', etc.
src/style.css
Outdated
font-family: Avenir; | ||
src: url(./fonts/Avenir-Book.ttf), url(./fonts/Avenir-Heavy.ttf), | ||
url(./fonts/Avenir.ttc); | ||
font-weight: 300; |
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 'font-weight' property is set to 300, but it's unclear if this matches the actual weights available in the 'Avenir-Book.ttf', 'Avenir-Heavy.ttf', and 'Avenir.ttc' files. Verify that the specified weight is supported by these font files.
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 implementing the search bars and styling! 🎉 While your code meets the basic requirements, there are a few areas for improvement:
- HTML Structure: Consider adding
<label>
elements for the input fields to enhance accessibility. Also, ensure that there are empty lines between multiline sibling blocks for better readability. - CSS Optimization: There are redundant styles in the hover and focus states for both
.big-search__input
and.small-search__input
. Simplifying these can make your CSS cleaner. Additionally, ensure thatbackground-size
andbackground-position
properties have the correct units to avoid rendering issues. - Font Loading: Specify the format for each font URL in the
@font-face
rule to ensure compatibility across browsers. - Styling Adjustments: The
font-weight
for the small search bar should be set to 300, and ensure there's a 20px gap between the search bars.
These adjustments will not only improve your code quality but also enhance user experience. Keep up the good work and continue learning from these insights! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
background: #fff; | ||
border: 1px solid #e1e7ed; |
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 hover state for .main__big-search
repeats the same background and border styles as the default state. Consider removing these redundant styles to simplify the CSS.
background-image: url(../images/Search.svg); | ||
background-repeat: no-repeat; | ||
background-size: 19px; | ||
background-position: 26px 24px; |
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 focus state for .big-search__input
repeats the background image properties that are already defined in the default state. Consider removing these redundant styles to simplify the CSS.
background-size: 11; | ||
background-position: 13 14; |
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 background-size
and background-position
properties are missing units (e.g., 'px'). Ensure that these values have the correct units to avoid potential rendering issues.
background: #fff; | ||
border: 1px solid #e1e7ed; |
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 hover state for .small-search__input
repeats the same background and border styles as the default state. Consider removing these redundant styles to simplify the CSS.
background-image: url(../images/Search.svg); | ||
background-repeat: no-repeat; | ||
background-size: 11px; | ||
background-position: 13px 14px; |
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 focus state for .small-search__input
repeats the background image properties that are already defined in the default state. Consider removing these redundant styles to simplify the CSS.
<form | ||
data-qa="big" | ||
class="main__big-search" | ||
> |
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.
Consider adding a <label>
element for the input field in the form with class main__big-search
. This will improve accessibility by providing a text label for screen readers.
<form | ||
data-qa="small" | ||
class="main__small-search" | ||
> |
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.
Consider adding a <label>
element for the input field in the form with class main__small-search
. This will improve accessibility by providing a text label for screen readers.
src: url(./fonts/Avenir-Book.ttf), url(./fonts/Avenir-Heavy.ttf), | ||
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.
The @font-face
rule should specify the format for each font URL to ensure proper loading across different browsers. Consider adding format descriptors like format('truetype')
or format('opentype')
.
Typical Mistakes
fromBEM
lesson theory are checked.In need some tips on how to make this better. This is close but not perfect and two tests still come as failed.