-
Notifications
You must be signed in to change notification settings - Fork 4k
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
add solution #4538
base: master
Are you sure you want to change the base?
add solution #4538
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job so far! 😊 However, there are a few areas that need to be addressed before we can approve the pull request. Please ensure that the SCSS file is compiled to CSS and correctly linked in the HTML. Also, consider updating the class names and placeholder text for better clarity. Keep up the good work! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
@@ -13,20 +13,32 @@ | |||
<title>Document</title> |
There was a problem hiding this comment.
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. It should describe the main content of the page. For example, if it's a search form, you could use 'Search Form' as a title.
src/index.html
Outdated
@@ -13,20 +13,32 @@ | |||
<title>Document</title> | |||
<link | |||
rel="stylesheet" | |||
href="style.css" | |||
href="style.scss" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The href attribute in the link tag should point to a CSS file, not a SCSS file. Browsers can't interpret SCSS files. You need to compile your SCSS to CSS first.
<input | ||
data-qa="keypress" | ||
type="text" | ||
placeholder="Try “Los Angeles“" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The placeholder text 'Try “Los Angeles“' is repeated in both input fields. It's better to have unique placeholder text for each input field to guide the user on what to input.
src/index.html
Outdated
data-qa="keypress" | ||
type="text" | ||
placeholder="Try “Los Angeles“" | ||
class="form__input form__input--big" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The class names 'form__input--big' and 'form__input--small' are not very descriptive. It's better to use class names that describe the content or functionality of the elements they are applied to, not their appearance. For example, you could use 'form__input--primary' and 'form__input--secondary'.
src/style.scss
Outdated
|
||
margin-top: 20px; | ||
padding: 0; | ||
font-family: Avenir, Arial, sans-serif; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the checklist, you should use fallback fonts - alternative font-family in case the main one doesn't work. So, in addition to Avenir
and Arial
, you should also include a generic font family like sans-serif
. This is a code quality issue.
src/style.scss
Outdated
display: flex; | ||
width: 100%; | ||
|
||
margin-top: 20px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checklist recommends being consistent with your margins. You've used margin-top
here, but it's better to stick to either top or bottom margins to avoid potential margin collapse. This is a code style issue.
src/style.scss
Outdated
} | ||
|
||
&--small { | ||
box-sizing: border-box; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've used box-sizing: border-box;
in both .form__input--big
and .form__input--small
. Since they share this property, it's more efficient to set it in the parent .form__input
class. This is a code quality issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on your code! There are a few minor improvements to be made, but they don't impact the overall functionality of your project. Keep in mind the points about consistency in margins, using the name
attribute for input fields, and the action
attribute in forms. Keep up the good work! 😄👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
/> | ||
<form | ||
data-qa="big" | ||
class="form" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good practice to use the action
attribute in a form
tag. Even if you are not sending the form data anywhere, you can set action="#"
to indicate that the form doesn't have a specific action page.
/> | ||
<form | ||
data-qa="small" | ||
class="form" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good practice to use the action
attribute in a form
tag. Even if you are not sending the form data anywhere, you can set action="#"
to indicate that the form doesn't have a specific action page.
data-qa="keypress" | ||
type="text" | ||
placeholder="Try “Los Angeles“" | ||
class="form__input form__input--primary" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to add name
attribute to your input fields. It's important for accessibility and form data submission.
type="text" | ||
data-qa="hover" | ||
placeholder="Try “Los Angeles“" | ||
class="form__input form__input--secondary" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to add name
attribute to your input fields. It's important for accessibility and form data submission.
padding: 0; | ||
margin: 20px 0; | ||
|
||
font-family: Avenir, Arial, Helvetica, sans-serif; |
There was a problem hiding this comment.
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 did it correctly in this line. Good job!
display: flex; | ||
width: 100%; | ||
padding: 0; | ||
margin: 20px 0; |
There was a problem hiding this comment.
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've added both top and bottom margins here. Try to stick to either top or bottom to avoid potential margin collapse.
.form__input { | ||
box-sizing: border-box; | ||
display: flex; | ||
width: 100%; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't fix container size (if there is no such a requirement). You've set the width to 100% here, which is a good practice as it allows the content to dictate the size of the container.
DEMO LINK
TEST REPORT LINK
Icon implemented using background-image CSS property
Inputs are written inside of 'form' tag with correctly passed attributes
All
Typical Mistakes
fromBEM
lesson theory are checked.Code follows all the Code Style Rules ❗️