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.

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

solution #3896

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

solution #3896

wants to merge 15 commits into from

Conversation

Mariana-VV
Copy link

@Mariana-VV Mariana-VV commented Dec 20, 2023

  • DEMO LINK

  • TEST REPORT LINK

  • [ x] Icon implemented using background-image CSS property

  • [x ] Inputs are written inside of 'form' tag with correctly passed attributes

  • [ x] All Typical Mistakes from BEM lesson theory are checked.

  • [x ] Code follows all the Code Style Rules ❗️

Copy link

@lerastarynets lerastarynets left a comment

Choose a reason for hiding this comment

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

Pls add links to deployed version of your project and deployed test report to the description of the pr (README file)

Copy link

@SanyaBratashchuk SanyaBratashchuk left a comment

Choose a reason for hiding this comment

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

It doesn't look like on design
image
If you faces with any difficulties feel free to ask in slack

Copy link

@maxim2310 maxim2310 left a comment

Choose a reason for hiding this comment

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

Good work, but lets make some improvement

1.need to check why you font family not applied
image
2. plz check one more time sizes and indents of icon, because on test as you can see it so different from referens

src/index.html Outdated
Comment on lines 15 to 16


Choose a reason for hiding this comment

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

Suggested change

plz remove all unnecessary spaces

src/index.html Outdated
Comment on lines 30 to 34
<input class="form__input--small"
type="text"
placeholder="Try “Los Angeles”"
data-qa="keypress"
>

Choose a reason for hiding this comment

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

Suggested change
<input class="form__input--small"
type="text"
placeholder="Try “Los Angeles”"
data-qa="keypress"
>
<input
class="form__input--small"
type="text"
placeholder="Try “Los Angeles”"
data-qa="keypress"
>

@Mariana-VV Mariana-VV requested a review from maxim2310 December 20, 2023 19:54
Copy link

@l4st1m0za l4st1m0za left a comment

Choose a reason for hiding this comment

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

Before sending the review again, try to pass all the tests as it's clear from there that you have mojor difference in your styles

@Mariana-VV Mariana-VV requested a review from l4st1m0za December 21, 2023 17:46
readme.md Outdated
- [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://<Mariana-VV>.github.io/layout_search-bar-airbnb/)
- [TEST REPORT LINK](https://<Mariana-VV>.github.io/layout_search-bar-airbnb/report/html_report/)

Choose a reason for hiding this comment

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

Suggested change
- [TEST REPORT LINK](https://<Mariana-VV>.github.io/layout_search-bar-airbnb/report/html_report/)
- [TEST REPORT LINK](https://Mariana-VV.github.io/layout_search-bar-airbnb/report/html_report/)

src/index.html Outdated
Comment on lines 28 to 33
<input
class="form__input--small"
type="text"
placeholder="Try “Los Angeles”"
data-qa="keypress"
>

Choose a reason for hiding this comment

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

Suggested change
<input
class="form__input--small"
type="text"
placeholder="Try “Los Angeles”"
data-qa="keypress"
>
<input
class="form__input--small"
type="text"
placeholder="Try “Los Angeles”"
data-qa="keypress"
>

src/style.css Outdated
Comment on lines 106 to 112
.form__input--big:focus {
background: linear-gradient(180deg, #fff 0%, #f6f6f7 100%);
}

.form__input--small:focus {
background: linear-gradient(180deg, #fff 0%, #f6f6f7 100%);
}

Choose a reason for hiding this comment

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

Suggested change
.form__input--big:focus {
background: linear-gradient(180deg, #fff 0%, #f6f6f7 100%);
}
.form__input--small:focus {
background: linear-gradient(180deg, #fff 0%, #f6f6f7 100%);
}
.form__input--big:focus,
.form__input--small:focus {
background: linear-gradient(180deg, #fff 0%, #f6f6f7 100%);
}

src/style.css Outdated
Comment on lines 97 to 103
.form__input--big:hover {
box-shadow: 0 4px 4px 0 rgba(0, 0, 0, 0.25);
}

.form__input--small:hover {
box-shadow: 0 4px 4px 0 rgba(0, 0, 0, 0.25);
}

Choose a reason for hiding this comment

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

same here

src/style.css Outdated
color: #3D4E61;
font-family: Avenir, sans-serif;
src: url(../src/fonts/Avenir.ttc);
font-size: 14px;

Choose a reason for hiding this comment

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

you forgot to add some spaces
image

src/style.css Outdated
Comment on lines 23 to 45
.form__input--big {
display: block;
width: 100%;
border-radius: 4px;
border: 1px solid #E1E7ED;
background: #FFF;
padding: 27px 63px;

box-shadow: 0 1px 7px 0 rgba(61, 78, 97, 0.10);

}

.form__input--small {
display: block;
width: 100%;

border-radius: 4px;
border: 1px solid #E1E7ED;
background: #FFF;
box-shadow: 0 1px 8px 0 rgba(61, 78, 97, 0.10);

padding:12px 32px;
}

Choose a reason for hiding this comment

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

create some general class form__input and move all duplicate styles there

src/style.css Outdated
Comment on lines 48 to 66
.form__icon--big {
background-image: url(images/Search.svg);
background-repeat: no-repeat;
position: absolute;
left: 27px;
top: 24px;
width: 19px;
height: 19px;

}

.form__icon--small {
background-image: url(images/Search.svg);
background-repeat: no-repeat;
position: absolute;
left: 14px;
top: 14px;
width: 11px;
height: 11px;

Choose a reason for hiding this comment

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

make it everywhere

Copy link

@loralevitska loralevitska left a comment

Choose a reason for hiding this comment

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

Looks like you have difference indents, please fix it.
image

If you have any questions, please ask them in fe_chat and tag mentors. It will be faster, because your answers in GitHub will be visible to mentor only after you re-request review

src/style.css Outdated
Comment on lines 18 to 20
position: relative;
margin: 20px 8px 10px;
padding: 0;

Choose a reason for hiding this comment

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

Suggested change
position: relative;
margin: 20px 8px 10px;
padding: 0;
position: relative;
margin: 20px 8px 10px;
padding: 0;

src/style.css Outdated
display: block;
width: 100%;
border-radius: 4px;
border: 1px solid #E1E7ED;

Choose a reason for hiding this comment

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

It is a good practice to make all colors variable

src/style.css Outdated
Comment on lines 41 to 42
background-image: url(images/Search.svg);
background-repeat: no-repeat;

Choose a reason for hiding this comment

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

Combine it with background property

src/style.css Outdated
font-family: Avenir, sans-serif;
src: url(../src/fonts/Avenir.ttc);
font-size: 14px;
font-style: normal;

Choose a reason for hiding this comment

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

Remove redundant default styles

Suggested change
font-style: normal;

Copy link

@etojeDenys etojeDenys left a comment

Choose a reason for hiding this comment

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

good job

Copy link

@volodymyr-soltys97 volodymyr-soltys97 left a comment

Choose a reason for hiding this comment

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

Good job 👍
Let's improve your code

  1. Need to remove the default border when input is focusing
image

src/index.html Outdated
placeholder="Try “Los Angeles”"
>

<span class="form__icon--big form__icon"></span>

Choose a reason for hiding this comment

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

Span is redundant here, add icon as a background image

Suggested change
<span class="form__icon--big form__icon"></span>

src/style.css Outdated
padding: 0;
}

.form:last-child(){

Choose a reason for hiding this comment

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

Suggested change
.form:last-child(){
.form:last-child {

src/style.css Outdated
src: url(./src/fonts/Avenir.ttc);
font-size: 14px;
font-weight: 300;
line-height: normal;

Choose a reason for hiding this comment

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

Default value, so you can just remove it

Suggested change
line-height: normal;

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

Great work! Consider fixing your layout: feel free to ask for help in the chat
Screenshot 2023-12-25 at 13 40 06
Also, consider implementing focus styles
Screenshot 2023-12-25 at 13 40 29

Copy link

@sTorba24 sTorba24 left a comment

Choose a reason for hiding this comment

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

Nicely done!

Check height of small input and use the right quotes i explained in fe_chat
Then you can pass all tests

@Mariana-VV Mariana-VV requested a review from sTorba24 December 25, 2023 14:52
font-size: 16px;
font-weight: 300;

font-family: Avenir, sans-serif;

Choose a reason for hiding this comment

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

set it for html and use inherit value


.form__input--big::placeholder {
font-size: 16px;
font-weight: 300;

Choose a reason for hiding this comment

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

for small the same, so set it for common form__input

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants