-
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
ad solution #4533
base: master
Are you sure you want to change the base?
ad solution #4533
Changes from 5 commits
efa03e7
8ce756a
3b2239f
107f12c
d44f3ff
141c968
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,23 +10,34 @@ | |
http-equiv="X-UA-Compatible" | ||
content="ie=edge" | ||
/> | ||
<title>Document</title> | ||
<title>Search Bar</title> | ||
<link | ||
rel="stylesheet" | ||
href="style.css" | ||
/> | ||
</head> | ||
<body> | ||
<input | ||
type="text" | ||
data-qa="keypress" | ||
placeholder="Try “Los Angeles“" | ||
/> | ||
|
||
<input | ||
type="text" | ||
data-qa="hover" | ||
placeholder="Try “Los Angeles“" | ||
/> | ||
<form | ||
class="search-bar search-bar-top" | ||
data-qa="big" | ||
> | ||
<input | ||
class="search-bar__input search-bar__input--primary" | ||
type="text" | ||
data-qa="keypress" | ||
placeholder="Try “Los Angeles“" | ||
/> | ||
</form> | ||
<form | ||
class="search-bar search-bar-bottom" | ||
data-qa="small" | ||
> | ||
<input | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above, consider renaming 'search-bar-input--bottom' to something more meaningful. |
||
class="search-bar__input search-bar__input--secondary" | ||
type="text" | ||
data-qa="hover" | ||
placeholder="Try “Los Angeles“" | ||
/> | ||
</form> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,67 @@ | ||
/* add styles here */ | ||
@font-face { | ||
font-family: Avenir; | ||
src: url(./fonts/Avenir-Book.ttf); | ||
font-weight: 300; | ||
} | ||
|
||
@font-face { | ||
font-family: Avenir-Heavy; | ||
src: url(./fonts/Avenir-Heavy.ttf); | ||
Comment on lines
+7
to
+8
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like you've declared a new font-family 'Avenir-Heavy'. Ideally, you should only declare the base font family once (i.e., 'Avenir') and then use the 'font-weight' property to control the boldness of the font. The browser will then choose the closest available font weight. This is important for the consistency and maintainability of your CSS code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The font-family name 'Avenir-Heavy' is not semantic. It would be better to use 'Avenir' as the font-family for all related fonts, and control the weight via the 'font-weight' property. This way, you can easily switch between font weights without having to change the font-family. |
||
font-weight: 900; | ||
} | ||
|
||
.search-bar { | ||
margin-top: 20px; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You've added a margin-top to the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's recommended to be consistent with your margins. Here you've added a top margin, but in a real-world scenario, it's better to stick to just one direction (either top or bottom) to avoid potential margin collapse problems. |
||
|
||
.search-bar-top { | ||
height: 70px; | ||
margin-top: 20px; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Be careful with fixing container size. In this case, you've fixed the height for '.search-bar-top'. Unless it's a specific requirement, it's better to let the content dictate the size of the container to avoid overflow or accidental scroll bar. |
||
|
||
.search-bar-bottom { | ||
height: 42px; | ||
margin-top: 20px; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same comment about fixing container size applies here for '.search-bar-bottom'. It's better to let the content size dictate the container size. |
||
} | ||
|
||
.search-bar__input { | ||
box-sizing: border-box; | ||
width: 100%; | ||
height: 100%; | ||
background-repeat: no-repeat; | ||
background-image: url(./images/Search.svg); | ||
border: 1px solid #e1e7ed; | ||
border-radius: 4px; | ||
box-shadow: 0 1px 8px 0 #3d4e611a; | ||
|
||
font-family: Avenir, sans-serif; | ||
font-weight: 300; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You've declared the 'Avenir' font here, but there's no fallback font. It's a good practice to always include a fallback font, in case the custom font doesn't load. For example, you can use a generic font family like 'sans-serif' as a fallback. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You've used the 'Avenir' font here, but it seems you've declared a new font-family 'Avenir-Heavy' at the beginning of your CSS. Remember to use the correct font-family name when applying it to your elements. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You have specified 'Avenir' as the font-family, but you haven't provided a fallback font. If the 'Avenir' font fails to load for any reason, the browser will use the default font, which may not fit your design. It's a good practice to specify a fallback font that is widely available across different systems. For example: |
||
color: #3d4e61; | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You've set the width and height of the |
||
.search-bar__input--primary { | ||
background-size: 19px 19px; | ||
background-position: 26px center; | ||
padding-left: 62px; | ||
font-size: 16px; | ||
} | ||
|
||
.search-bar__input--secondary { | ||
background-size: 11px 11px; | ||
background-position: 13px center; | ||
padding-left: 33px; | ||
font-size: 14px; | ||
} | ||
|
||
.search-bar__input::placeholder { | ||
color: #3d4e61; | ||
} | ||
|
||
.search-bar__input:hover { | ||
box-shadow: 0 1px 8px 0 rgba(0, 0, 0, 0.25); | ||
} | ||
|
||
.search-bar__input:focus { | ||
box-shadow: 0 1px 8px 0 rgba(0, 0, 0, 0.25); | ||
outline: none; | ||
} |
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 class name 'search-bar-input--top' and 'search-bar-input--bottom' do not represent the meaning of the content. Consider renaming these classes to something more meaningful. For example, if the class is intended to style the input differently based on its position, a name like 'search-bar-input-upper' and 'search-bar-input-lower' might be more appropriate.