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

ad solution #4533

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

ad solution #4533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
efa03e7
ad solution
t-namestnykova Oct 11, 2024
8ce756a
solution
t-namestnykova Oct 11, 2024
3b2239f
add changes to readme.md
t-namestnykova Oct 11, 2024
107f12c
solution
t-namestnykova Oct 11, 2024
d44f3ff
add solution
t-namestnykova Oct 11, 2024
141c968
solution with changes
t-namestnykova Oct 11, 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://t-namestnykova.github.io/layout_search-bar-airbnb/)
- [TEST REPORT LINK](https://t-namestnykova.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
35 changes: 23 additions & 12 deletions src/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link

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.

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

Choose a reason for hiding this comment

The 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>
62 changes: 61 additions & 1 deletion src/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,61 @@
/* 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
Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

The 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-top,
.search-bar-bottom {
margin-top: 20px;
}

.search-bar__input {
box-sizing: border-box;
width: 100%;
padding: 0;
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;
Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

The 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: font-family: Avenir, Arial, sans-serif;

color: #3d4e61;
}

Copy link

Choose a reason for hiding this comment

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

You've set the width and height of the .search-bar-input class to 100%. This can cause issues if the parent container's size is not fixed. It's generally better not to fix container size, and let the content size dictate it. This helps to avoid overflow or accidental scroll bar. (Refer to the CSS checklist)

.search-bar__input--primary {
background-size: 19px 19px;
background-position: 26px center;
padding-left: 62px;
font-size: 16px;
height: 70px;
}

.search-bar__input--secondary {
background-size: 11px 11px;
background-position: 13px center;
padding-left: 33px;
font-size: 14px;
height: 42px;
}

.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;
font-family: Avenir-Heavy, sans-serif;
}
Copy link

Choose a reason for hiding this comment

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

Same as before, remember to provide a fallback font-family.

Loading