-
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 #4545
base: master
Are you sure you want to change the base?
add task solution #4545
Changes from all commits
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,41 @@ | |
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 | ||
action="#" | ||
data-qa="big" | ||
> | ||
<label for="search-input"> | ||
<input | ||
id="search-input" | ||
class="search-bar search-bar--big" | ||
type="text" | ||
data-qa="keypress" | ||
placeholder="Try “Los Angeles“" | ||
/> | ||
Comment on lines
+26
to
+32
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. Keep your attributes correctly formatted. If the HTML-element has long attribute values or number of attributes is more than 2 - start each one, including the first, on the new line with 2-space indentation related to tag. Tag’s closing bracket should be on the same level as opening one. |
||
</label> | ||
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. As per HTML Content checklist, it's recommended to use semantic tags where possible. In this case, 'label' tag is used without a 'for' attribute and without any text, which doesn't provide any semantic value. Consider removing the 'label' tag if it's not necessary, or provide a 'for' attribute linking to the 'id' of the input field.
Comment on lines
+25
to
+33
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. Remember about correct indentation between parent and child elements. Each level of nesting, including text, contained inside the element, requires 2-space offset. Also blank line shouldn't be between parent and child elements. |
||
</form> | ||
Comment on lines
+21
to
+34
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. According to the checklist, you should add empty lines between multiline sibling blocks of HTML. This will improve readability. |
||
<form | ||
action="#" | ||
data-qa="small" | ||
> | ||
<label for="search-input2"> | ||
<input | ||
id="search-input2" | ||
class="search-bar search-bar--small" | ||
type="text" | ||
data-qa="hover" | ||
placeholder="Try “Los Angeles“" | ||
/> | ||
</label> | ||
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 issue here as in line 32. Please revise your usage of 'label' tag. |
||
</form> | ||
</body> | ||
</html> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,81 @@ | ||
/* add styles here */ | ||
@font-face { | ||
font-family: 'Avenir Book'; | ||
src: url('./fonts/Avenir-Book.ttf') format('truetype'); | ||
font-weight: 300; | ||
} | ||
|
||
@font-face { | ||
font-family: 'Avenir Heavy'; | ||
src: url('./fonts/Avenir-Heavy.ttf') format('truetype'); | ||
font-weight: bold; | ||
} | ||
|
||
body { | ||
font-family: Avenir, Arial, Helvetica, sans-serif; | ||
} | ||
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. Remember to use fallback fonts - alternative font-family in case the main one doesn't work. You can add 'Arial, Helvetica, sans-serif' as fallback fonts for 'Avenir'. |
||
|
||
.search-bar { | ||
font-family: Avenir, sans-serif; | ||
width: 100%; | ||
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 defined the font-family for the class 'search-bar' as 'Avenir'. However, you have not defined a font-face for 'Avenir'. You should either define a font-face for 'Avenir' or use one of the fonts you have defined, like 'Avenir Book' or 'Avenir Heavy'. 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. Remember to use fallback fonts - alternative font-family in case the main one doesn't work. You can add 'Arial, Helvetica, sans-serif' as fallback fonts for 'Avenir'. |
||
display: block; | ||
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. Avoid fixing container size if there is no such requirement. Let the content size dictate it. To avoid overflow or accidental scroll bar. |
||
margin-top: 20px; | ||
background: #fff; | ||
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 added a top margin to the 'search-bar' class. This is not a good practice as per the checklist. Instead of adding margins to the top and bottom, it is recommended to add them to either the top or the bottom to avoid potential margin collapse. |
||
border: 1px solid #e1e7ed; | ||
border-radius: 5px; | ||
box-shadow: 0 1px 8px 0 #3d4e611a; | ||
box-sizing: border-box; | ||
} | ||
|
||
.search-bar--big { | ||
height: 70px; | ||
font-size: 16px; | ||
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 fixed the height of the 'search-bar__big' class. Unless there is a requirement, it is recommended not to fix the container size. Let the content size dictate it to avoid overflow or accidental scroll bars. |
||
padding-left: 62px; | ||
background-image: url(images/Search.svg); | ||
background-repeat: no-repeat; | ||
background-size: 19px; | ||
background-position: left 26px center; | ||
background-color: #fff; | ||
} | ||
|
||
.search-bar--small { | ||
height: 42px; | ||
font-size: 14px; | ||
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. Similar to the 'search-bar__big' class, you have also fixed the height of the 'search-bar__small' class. It is recommended not to fix the container size unless there is a requirement. |
||
padding-left: 33px; | ||
background-image: url(images/Search.svg); | ||
background-repeat: no-repeat; | ||
background-size: 11px; | ||
background-position: left 13px center; | ||
background-color: #fff; | ||
} | ||
|
||
.search-bar:hover { | ||
box-shadow: 0 3px 8px 0 #3d4e6133; | ||
} | ||
|
||
.search-bar--big:focus { | ||
font-weight: bold; | ||
background: | ||
url('./images/Search.svg') no-repeat, | ||
linear-gradient(180deg, transparent, #f6f6f7); | ||
background-size: 19px; | ||
background-position: left 26px center; | ||
border: 1px solid #e1e7ed; | ||
box-shadow: 0 4px 4px 0 #00000040; | ||
outline: none; | ||
} | ||
|
||
.search-bar--small:focus { | ||
font-weight: bold; | ||
background: | ||
url('./images/Search.svg') no-repeat, | ||
linear-gradient(180deg, transparent, #f6f6f7); | ||
background-size: 11px; | ||
background-position: left 13px center; | ||
border: 1px solid #e1e7ed; | ||
box-shadow: 0 4px 4px 0 #00000040; | ||
outline: none; | ||
} | ||
|
||
.search-bar::placeholder { | ||
color: #3d4e61; | ||
} |
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 'for' attribute of the label element must be equal to the id of a non-hidden form control. Please add an 'id' attribute to the input and make sure it matches with 'for' attribute of the label. This is an accessibility issue.