Skip to content
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.

Sign up for GitHub

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

Jump to bottom

add task solution #4479

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

add task solution #4479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a5e6ea
add task solution
izotovav Sep 21, 2024
fc8b1bb
add the first patch
izotovav Sep 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 24 additions & 10 deletions src/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,30 @@
/>
</head>
<body>
<input
type="text"
data-qa="keypress"
placeholder="Try “Los Angeles“"
/>
<header class="search-container">
<form
class="search-bar top-search-bar"
data-qa="big"
>
<input
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The alt attribute is missing on the input element. Although it's not required for input elements, if you had images, remember to provide meaningful alt text for images.

class="search-bar-input search-bar-input--top"
type="text"
data-qa="keypress"
placeholder="Try “Los Angeles“"
Comment on lines +21 to +29
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The attributes in the input tag should start on a new line with 2-space indentation relative to the tag, and the closing bracket should be on the same level as the opening one. Please adjust the formatting of the input tag to match the guideline.

/>
</form>
Comment on lines +20 to +31
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding an empty line between the header tag and the form tag to add some 'air' and simplify reading, as they are sibling elements. However, do not add an empty line between the form and its child input as they are parent and child elements.


<input
type="text"
data-qa="hover"
placeholder="Try “Los Angeles“"
/>
<form
class="search-bar bottom-search-bar"
data-qa="small"
>
<input
class="search-bar-input search-bar-input--bottom"
type="text"
data-qa="hover"
placeholder="Try “Los Angeles“"
/>
Comment on lines +33 to +42
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The attributes in the input tag should start on a new line with 2-space indentation relative to the tag, and the closing bracket should be on the same level as the opening one. Please adjust the formatting of the input tag to match the guideline.

</form>
</header>
</body>
</html>
74 changes: 73 additions & 1 deletion src/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,73 @@
/* add styles here */
@font-face {
font-family: Avenir-Book;
src: url(./fonts/Avenir-Book.ttf);
}

@font-face {
font-family: Avenir-Heavy;
src: url(./fonts/Avenir-Heavy.ttf);
font-weight: 900;
}

* {
font-family: Avenir-Book, Arial, Helvetica, sans-serif;
font-weight: 300;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remember to use fallback fonts. It's good you have Arial and Helvetica, but you should also include a generic family like 'sans-serif' at the end.

font-style: normal;
color: rgba(61, 78, 97, 1);
box-sizing: border-box;
}
Comment on lines +12 to +17
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The '' selector impacts performance and should not be used. It's better to set styles specifically for elements that require them. Also, avoid setting global font styles on the '' selector; use 'body' or specific classes instead.

Comment on lines +12 to +17
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid using the * selector as it impacts performance. It's better to set styles only for elements that require them.


body {
margin-top: 0;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the checklist, be consistent with your margins by adding only top or bottom, but not both. It seems you have set the top margin here, ensure that you're not also setting the bottom margin elsewhere for the same element to avoid potential margin collapse.


Comment on lines +20 to +22
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Be consistent with your margins. Add only top or bottom margins, but not both, to avoid potential margin collapse issues.

.search-bar {
margin-top: 20px;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the same principle as the previous comment, ensure that you're not setting both top and bottom margins for elements to avoid potential margin collapse issues.


Comment on lines +24 to +26
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have a margin-top on the .search-bar class. If there are no specific requirements for the container size, consider letting the content size dictate the spacing instead of fixing it with margins.

.search-bar-input {
width: 100%;
height: 100%;
background-image: url(images/Search.svg);
background-repeat: no-repeat;
border-radius: 4px;
border: 1px solid #e1e7ed;
box-shadow: 0 1px 8px 0 #3d4e611a;
box-sizing: border-box;
}

Comment on lines +28 to +37
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid fixing container sizes like width and height unless required. Let the content size dictate the container size to avoid overflow or accidental scroll bars.

.top-search-bar {
height: 70px;
}

.bottom-search-bar {
height: 42px;
}

.search-bar-input--top {
padding-left: 62px;
background-position: 26px center;
background-size: 19px 19px;
font-size: 16px;
}

.search-bar-input--bottom {
padding-left: 33px;
background-position: top 15px left 13px;
background-size: 11px 11px;
font-size: 14px;
}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is recommended not to fix container sizes unless there's a specific requirement for it. Ensure that the fixed heights for the '.search-bar-input--top' and '.search-bar-input--bottom' are indeed necessary and that the content does not overflow or create an accidental scrollbar.

.search-bar-input::placeholder {
color: #3d4e61;
}

.search-bar-input:hover {
box-shadow: 0 3px 8px 0 #3d4e6133;
}

.search-bar-input:focus {
outline: none;
font-family: Avenir-Heavy, Arial, Helvetica, sans-serif;
box-shadow: 0 4px 4px 0 #00000040;
}
Loading