-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 task solution #4737
base: master
Are you sure you want to change the base?
add task solution #4737
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 pull request! 😊 While your code meets most of the task requirements, there are a few critical issues that need to be addressed before we can approve it:
-
Font Weight: Ensure the default
font-weight
is set to 300 as required. This is crucial for maintaining the design specifications. -
CSS Selector Usage: Avoid using the universal selector
*
as it can negatively impact performance. This is important for ensuring the code remains efficient and maintainable. -
HTML Formatting: Consistency in HTML indentation is essential for readability and maintainability. Please ensure all elements are indented with 2 spaces.
These issues are important to resolve to ensure the functionality and maintainability of the code. Once these are addressed, we can move forward with the approval. Keep up the great work, and I'm confident you'll have this ready in no time! 🚀
✨ 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
<form | ||
class="search__form search__form--big" | ||
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.
Consider adding a method
attribute to the form element to specify how the form data should be submitted. Common values are GET
or POST
.
src/index.html
Outdated
<form | ||
class="search__form search__form--small" | ||
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.
Similar to the previous form, consider adding a method
attribute to specify how the form data should be submitted.
src/index.html
Outdated
<input | ||
class="search__input search__input--big" | ||
type="text" | ||
data-qa="keypress" | ||
placeholder="Try “Los Angeles“" | ||
/> |
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 would be beneficial to add a name
attribute to the input element to ensure that the data can be properly identified when the form is submitted.
src/index.html
Outdated
<input | ||
class="search__input search__input--small" | ||
type="text" | ||
data-qa="hover" | ||
placeholder="Try “Los Angeles“" | ||
/> |
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 name
attribute to this input element as well, to ensure the data can be properly identified upon form submission.
/* add styles here */ | ||
@font-face { | ||
font-family: Avenir; | ||
src: url(fonts/Avenir-Book.ttf); |
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 providing multiple font formats (e.g., WOFF, WOFF2) for better cross-browser compatibility. This ensures that the font is displayed correctly across different browsers.
|
||
.search__input { | ||
width: 100%; | ||
background-image: url(/src/images/Search.svg); |
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.
Ensure that the image path is correct and that the image exists at the specified location. This will prevent broken image links in the background.
.search__input--big { | ||
width: 100%; | ||
height: 70px; | ||
padding-left: 62px; /* 26px відступ для іконки + 17px текст після іконки + 19px ширина іконки */ |
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 comment here is in Ukrainian. Consider translating it to English for consistency and to ensure that all team members can understand it.
.search__input--small { | ||
width: 100%; | ||
height: 42px; | ||
padding-left: 33px; /* це 13px відступ для іконки + 9px текст після іконки + 11px ширина іконки */ |
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.
Similar to the previous comment, consider translating this comment to English for consistency and clarity.
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.
Sorry, the pull request review was unsuccessful for the following reasons:
Warning
No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: 7]
If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.
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.
Review Summary
Great job on addressing most of the issues from the previous review! 🎉 Your HTML and CSS files are generally well-structured and follow good practices. Here are some points to consider for further improvement:
-
HTML Formatting: You've successfully ensured consistent 2-space indentation throughout the HTML file, which enhances readability and maintainability. Keep up the good work!
-
CSS Improvements:
- Font Weight: Well done on setting the default
font-weight
to 300 as required by the task description. This is crucial for maintaining the design specifications. - Universal Selector: The universal selector
*
is still being used in the CSS file. It's important to avoid this as it can negatively impact performance. Consider applying styles to specific elements or classes instead.
- Font Weight: Well done on setting the default
-
Maintainability: Ensure consistent indentation for the HTML elements within the
<div class="search">
block to further improve readability and maintainability.
While the universal selector issue remains unresolved, the decision to approve your Pull Request is made to help you move forward with your learning. Please review the comments to improve your code. Addressing these issues will enhance the performance and maintainability of your project. 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! ✨
<div class="search"> | ||
<form | ||
method="get" | ||
class="search__form search__form--big" | ||
data-qa="big" | ||
> | ||
<input | ||
class="search__input search__input--big" | ||
name="search-bar-big" | ||
type="text" | ||
data-qa="keypress" | ||
placeholder="Try “Los Angeles“" | ||
/> | ||
</form> | ||
|
||
<input | ||
type="text" | ||
data-qa="hover" | ||
placeholder="Try “Los Angeles“" | ||
/> | ||
<form | ||
method="get" | ||
class="search__form search__form--small" | ||
data-qa="small" | ||
> | ||
<input | ||
class="search__input search__input--small" | ||
name="search-bar-small" | ||
type="text" | ||
data-qa="hover" | ||
placeholder="Try “Los Angeles“" | ||
/> | ||
</form> | ||
</div> |
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.
Ensure consistent indentation for the HTML elements within the <div class="search">
block. This will improve readability and maintainability.
*, | ||
*::before, | ||
*::after { | ||
box-sizing: border-box; | ||
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.
Avoid using the universal selector *
as it can negatively impact performance. Consider applying styles to specific elements or classes instead.
*::before, | ||
*::after { | ||
box-sizing: border-box; | ||
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.
Ensure the default font-weight
is set to 300 as required by the task description. This is crucial for maintaining the design specifications.
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 ❗️