-
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 task solution #4544
base: master
Are you sure you want to change the base?
add task solution #4544
Conversation
RomanHasiuk
commented
Oct 16, 2024
- DEMO LINK
- TEST REPORT LINK
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 CSS file! There are a few areas that need some attention though. 😊 Please correct the typos in your class names and consider the comments about fixed container sizes and the usage of the *
selector. Also, don't forget to give your page a meaningful title. Keep up the good work! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
class="search-bar search-bar-big" | ||
data-qa="big" | ||
> | ||
<label class="search-bar__label search-bar__lable-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.
There is a typo in the class name 'search-bar__lable-big'. It should be 'search-bar__label-big'.
src/index.html
Outdated
class="search-bar search-bar-small" | ||
data-qa="small" | ||
> | ||
<label class="search-bar__label search-bar__lable-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.
There is a typo in the class name 'search-bar__lable-small'. It should be 'search-bar__label-small'.
src/style.css
Outdated
* { | ||
padding: 0; | ||
box-sizing: border-box; | ||
} |
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 use of the *
selector is discouraged because it impacts performance. It is more efficient to set styles only for elements that require them.
src/style.css
Outdated
} | ||
|
||
.page { | ||
font-family: Avenir, sans-serif; |
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.
Good job on including a fallback font with the font-family
property. This ensures that if the primary font fails to load, the text will still be displayed in a different font.
src/style.css
Outdated
.search-bar { | ||
display: flex; | ||
justify-content: center; | ||
margin: 20px auto 0; |
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's a good practice to be consistent with your margins. Here you're using margin
to add space at the top of the .search-bar
. Make sure you're not adding bottom margins elsewhere in your code to avoid potential margin collapse.
src/style.css
Outdated
|
||
.search-bar-big { | ||
width: 100%; | ||
height: 70px; |
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 fixing container sizes unless it's a requirement. Here, you're setting a fixed height for .search-bar-big
. Let the content size dictate the container size to avoid overflow or accidental scroll bars.
src/style.css
Outdated
|
||
.search-bar-small { | ||
width: 100%; | ||
height: 42px; |
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, avoid fixing container sizes unless it's a requirement. Here, you're setting a fixed height for .search-bar-small
. Let the content size dictate the container size to avoid overflow or accidental scroll bars.
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 the submission! 🎉 While there are some minor formatting and style issues, they don't critically impact the functionality of your code. Keep these suggestions in mind for future improvements, and continue honing your skills!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
<html | ||
lang="en" | ||
class="page" | ||
> |
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 HTML tag should not have attributes split across multiple lines unless there are more than two attributes or the attributes are long. Consider placing them on a single line.
src/index.html
Outdated
|
||
<input | ||
type="text" | ||
data-qa="hover" | ||
placeholder="Try “Los Angeles“" | ||
/> | ||
<body class="page__body"> |
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.
There should be no blank line between the head and body tags. Remove the empty line to maintain proper HTML formatting.
<form | ||
class="search-bar search-bar-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.
The attributes for the form tag are correctly formatted since there are more than two attributes. However, ensure that the line length does not exceed 80 characters.
<input | ||
data-qa="keypress" | ||
class="search-bar__input search-bar__input-big" | ||
type="text" | ||
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.
The attributes for the input tag are correctly formatted since there are more than two attributes. However, ensure that the line length does not exceed 80 characters.
src/index.html
Outdated
|
||
<form |
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.
There should be an empty line between the sibling form elements to improve readability.
<form | ||
class="search-bar search-bar-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.
The attributes for the form tag are correctly formatted since there are more than two attributes. However, ensure that the line length does not exceed 80 characters.
<input | ||
data-qa="hover" | ||
class="search-bar__input search-bar__input-small" | ||
type="text" | ||
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.
The attributes for the input tag are correctly formatted since there are more than two attributes. However, ensure that the line length does not exceed 80 characters.
src/style.css
Outdated
@@ -1 +1,124 @@ | |||
/* add styles here */ | |||
* { |
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.
Using the universal selector '*' is not recommended as it impacts performance. Consider applying styles directly to specific elements or classes that require them.
src/style.css
Outdated
.search-bar__input-big { | ||
padding-left: 62px; | ||
font-size: 16px; | ||
line-height: 22; |
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 'line-height' property should have a unit specified, such as 'px'. Without a unit, it may not render as expected.
src/style.css
Outdated
.search-bar__input-small { | ||
padding-left: 33px; | ||
font-size: 14px; | ||
line-height: 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 'line-height' property should have a unit specified, such as 'px'. Without a unit, it may not render as expected.
…the universal selector