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 #4545

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

add task solution #4545

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d2c717d
add task solution
veronikamaliar Oct 16, 2024
f994f5d
add task solution
veronikamaliar Oct 16, 2024
045114e
add task solution
veronikamaliar Oct 16, 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
4 changes: 2 additions & 2 deletions readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ ___

❗️ Replace `<your_account>` with your Github username and copy the links to `Pull Request` description:

- [DEMO LINK](https://<your_account>.github.io/layout_search-bar-airbnb/)
- [TEST REPORT LINK](https://<your_account>.github.io/layout_search-bar-airbnb/report/html_report/)
- [DEMO LINK](https://veronikamaliar.github.io/layout_search-bar-airbnb/)
- [TEST REPORT LINK](https://veronikamaliar.github.io/layout_search-bar-airbnb/report/html_report/)

❗️ Copy this `Checklist` to the `Pull Request` description after links, and put `- [x]` before each point after you checked it.

Expand Down
38 changes: 27 additions & 11 deletions src/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,38 @@
content="ie=edge"
/>
<title>Document</title>
Copy link

Choose a reason for hiding this comment

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

It is advisable to give your HTML document a more descriptive title, rather than the default 'Document'. This is important for SEO and for better understanding of the page content.

Copy link

Choose a reason for hiding this comment

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

The 'title' tag should be more descriptive about the content of the page. This is an SEO issue.


<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>
<input
class="search-bar search-bar__big"
Copy link

Choose a reason for hiding this comment

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

It looks like you have two classes assigned to the same element. If 'search-bar' is a block, it should not be a child of itself ('search-bar__big'). Please consider revising your class naming to adhere to BEM methodology.

type="text"
data-qa="keypress"
placeholder="Try “Los Angeles“"
/>
</label>
Copy link

Choose a reason for hiding this comment

The 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.

</form>
<form
action="#"
data-qa="small"
>
<label>
<input
class="search-bar search-bar__small"
Copy link

Choose a reason for hiding this comment

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

Same issue here as in line 27. Please revise your class naming.

type="text"
data-qa="hover"
placeholder="Try “Los Angeles“"
/>
</label>
Copy link

Choose a reason for hiding this comment

The 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>
82 changes: 81 additions & 1 deletion src/style.css
View file
Open in desktop
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 Book', sans-serif;
}
Copy link

Choose a reason for hiding this comment

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

You have used a fallback font for the body tag, which is good practice. However, you should consider adding more fallback fonts. For instance, you can add 'Arial', 'Helvetica' as fallbacks. This will ensure that if 'Avenir Book' doesn't load, the browser will try to load 'Arial', then 'Helvetica', and finally 'sans-serif'.


.search-bar {
font-family: Avenir, sans-serif;
width: 100%;
Copy link

Choose a reason for hiding this comment

The 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'.

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 - 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;
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 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;
Copy link

Choose a reason for hiding this comment

The 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;
Copy link

Choose a reason for hiding this comment

The 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;
Copy link

Choose a reason for hiding this comment

The 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;
}
Loading