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

Develop #4532

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

Develop #4532

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fccf024
add task solution
Bankir4ik87 Oct 9, 2024
5ea124e
add fix
Bankir4ik87 Oct 10, 2024
dc48a15
add fix1
Bankir4ik87 Oct 10, 2024
4ab40f8
add task solution
Bankir4ik87 Oct 11, 2024
27890fd
add fix mentor`s review
Bankir4ik87 Oct 11, 2024
6618d64
add fix1 mentor`s review
Bankir4ik87 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://Bankir4ik87.github.io/layout_search-bar-airbnb/)
- [TEST REPORT LINK](https://Bankir4ik87.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
32 changes: 22 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,28 @@
/>
</head>
<body>
<input
type="text"
data-qa="keypress"
placeholder="Try “Los Angeles“"
/>
<form
class="search-bar primary-search-bar"
data-qa="big"
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 class names search-bar and primary-search-bar here. Class names should be semantic and describe the meaning of the content, not the styles or tag names. Consider renaming primary-search-bar to something more descriptive of its function or content, for example main-search-bar.

>
<input
class="search-bar-input search-bar-input--primary"
type="text"
data-qa="keypress"
placeholder="Try “Los Angeles“"
Copy link

Choose a reason for hiding this comment

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

Good job using the placeholder attribute for user guidance. However, consider making the placeholder text more generic and not location-specific. Something like 'Search...' could work better.

/>
</form>

<input
type="text"
data-qa="hover"
placeholder="Try “Los Angeles“"
/>
<form
class="search-bar secondary-search-bar"
data-qa="small"
Copy link

Choose a reason for hiding this comment

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

Same comment as above for secondary-search-bar. A more descriptive name like additional-search-bar would be better.

>
<input
class="search-bar-input search-bar-input--secondary"
type="text"
data-qa="hover"
placeholder="Try “Los Angeles“"
Copy link

Choose a reason for hiding this comment

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

Same comment as above for the placeholder text.

/>
</form>
</body>
</html>
72 changes: 71 additions & 1 deletion src/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,71 @@
/* add styles here */
body {
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.

It's better to specify a fallback font in case the main one doesn't load. You can add a generic font family such as 'sans-serif' to the end of your font stack. Also, be sure to include 'Avenir-Book' and 'Avenir-Heavy' in your font stack if you're using them elsewhere in your CSS.

Copy link

Choose a reason for hiding this comment

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

This line doesn't provide fallback fonts. Remember to use fallback fonts - alternative font-family in case the main one doesn't work. In case 'Avenir' font fails to load, the browser will use the generic 'sans-serif' font. However, it would be better to specify one or two more specific fallback fonts before resorting to the generic 'sans-serif'. For example, 'Helvetica' and 'Arial' are commonly used fallbacks for 'Avenir'. (CHECKLIST: Remember to use fallback fonts - alternative font-family in case the main one doesn't work)

color: #3d4e61;
}

@font-face {
font-family: Avenir-Book;
font-style: normal;
font-weight: normal;
src: url(./fonts/Avenir-Book.ttf);
}

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

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

According to the checklist, it's better to be consistent with your margins. You should only add top or bottom margins, but not both. Here, you're adding a top margin, which may lead to unexpected layout shifts if other elements also have bottom margins.

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. You have used top margin here. Stick to either top or bottom margins for consistency and to avoid potential margin collapses. (CHECKLIST: Be consistent with your margins (Add only top or bottom, but not both))

Copy link

Choose a reason for hiding this comment

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

You have set the top margin for the '.search-bar' class. However, it would be more consistent if you add margins to the bottom of elements, not to the top. This helps to avoid potential margin collapse. (Checklist: Be consistent with your margins (Add only top or bottom, but not both))


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

The same comment about margins applies here. It's better to be consistent and only add top or bottom margins, but not both.

Copy link

Choose a reason for hiding this comment

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

Again, you have used top margin here. As mentioned before, it's better to stick to either top or bottom margins for consistency and to avoid potential margin collapses. (CHECKLIST: Be consistent with your margins (Add only top or bottom, but not both))

Copy link

Choose a reason for hiding this comment

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

You have set the top margin for the '.additional-search-bar' class. As mentioned above, it would be more consistent if you add margins to the bottom of elements, not to the top. (Checklist: Be consistent with your margins (Add only top or bottom, but not both))


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

Choose a reason for hiding this comment

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

It's better to use classes for styling instead of tag names, unless the tag is 'html' or 'body'. Here, you're styling the 'input' tag directly, which could cause conflicts if you have other 'input' elements on your page that need different styles.

}

.search-bar-input--primary {
font-family: Avenir-Book, sans-serif;
height: 70px;
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' property in the '.search-bar-input--main' class should include fallback fonts. (Checklist: Remember to use fallback fonts - alternative font-family in case the main one doesn't work)

box-sizing: border-box;
background-size: 19px 19px;
background-position: 26px center;
padding-left: 62px;
font-size: 16px;
line-height: 21.86px;
}

.search-bar-input--secondary {
font-family: Avenir-Book, sans-serif;
height: 42px;
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' property in the '.search-bar-input--additional' class should include fallback fonts. (Checklist: Remember to use fallback fonts - alternative font-family in case the main one doesn't work)

box-sizing: border-box;
background-size: 11px 11px;
background-position: 13px center;
padding-left: 33px;
font-size: 14px;
line-height: 19.12px;
}

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

.search-bar-input:hover {
box-shadow: rgba(0, 0, 0, 0.25);
}

.search-bar-input:focus {
box-shadow: rgba(0, 0, 0, 0.25);
font-family: Avenir-Heavy, sans-serif;
outline: none;
Copy link

Choose a reason for hiding this comment

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

You're changing the 'font-family' on focus, which could cause a noticeable and potentially disorienting change for the user. It's better to keep the 'font-family' consistent across different states.

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' property in the '.search-bar-input:focus' class should include fallback fonts. (Checklist: Remember to use fallback fonts - alternative font-family in case the main one doesn't work)

}
Loading