Develop #4536
Develop #4536
Conversation
There was a problem hiding this comment.
Great work so far! However, there are a few things that need to be addressed. Make sure to correct the usage of the font-family property in the @font-face rule and specify the fallback font-families. Also, please review the usage of the flex-direction property and the body class. Keep going, you're doing well! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/index.html
Outdated
| <body class="body"> | ||
| <main class="main"> | ||
| <form class="search-bar"> | ||
| <label for="id1"></label> |
There was a problem hiding this comment.
HTML Content: Labels should always have some text. If you want to hide it visually, but make it available for screen readers, you can use a technique like this: <label class="visually-hidden" for="id1">Search</label>. Class visually-hidden should be styled appropriately in CSS.
src/index.html
Outdated
| <input | ||
| id="id1" | ||
| type="text" | ||
| data-qa="big" | ||
| action="#" | ||
| placeholder="Try “Los Angeles“" | ||
| class="search-bar__big input" | ||
| /> |
There was a problem hiding this comment.
HTML Formatting: If the HTML-element has long attribute values or number of attributes is more than 2 - start each one, including the first, on the new line with 2-space indentation related to tag. Tag’s closing bracket should be on the same level as opening one.
src/index.html
Outdated
| id="id1" | ||
| type="text" | ||
| data-qa="big" | ||
| action="#" |
There was a problem hiding this comment.
HTML Content: The action attribute is not applicable to the input element. It should be used within the form tag to specify the URL of the server-side script that will process the submitted form data.
src/index.html
Outdated
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| <label for="id2"></label> |
There was a problem hiding this comment.
HTML Content: Labels should always have some text. If you want to hide it visually, but make it available for screen readers, you can use a technique like this: <label class="visually-hidden" for="id2">Search</label>. Class visually-hidden should be styled appropriately in CSS.
src/index.html
Outdated
| <input | ||
| id="id2" | ||
| type="text" | ||
| data-qa="small" | ||
| action="#" | ||
| placeholder="Try “Los Angeles“" | ||
| class="search-bar__small input" | ||
| /> |
There was a problem hiding this comment.
HTML Formatting: If the HTML-element has long attribute values or number of attributes is more than 2 - start each one, including the first, on the new line with 2-space indentation related to tag. Tag’s closing bracket should be on the same level as opening one.
src/style.css
Outdated
| .body { | ||
| margin: 0; | ||
| padding: 0 8px; | ||
| font-family: Avenir, Arial, Helvetica, sans-serif; |
There was a problem hiding this comment.
Here you should specify the fallback font-families. The correct usage would be font-family: Avenir, Arial, Helvetica, sans-serif; if Avenir is the name you've defined in the @font-face rule.
src/style.css
Outdated
| .body { | ||
| margin: 0; | ||
| padding: 0 8px; | ||
| font-family: Avenir, Arial, Helvetica, sans-serif; | ||
| } |
There was a problem hiding this comment.
It seems like you're trying to style the body element, but you're using a class selector .body. If you want to style the body tag, you should use body without the dot. If you meant to style a class, make sure the class body is applied in your HTML.
src/style.css
Outdated
| .search-bar { | ||
| display: flex; | ||
| flex-direction: column; | ||
| } |
There was a problem hiding this comment.
I noticed that you're using a flex-direction: column; property for your .search-bar class, but I don't see any child elements inside this class in your CSS. If there are no child elements to arrange in a column, you don't need this property.
src/style.css
Outdated
| .search-bar__big:focus { | ||
| color: #3d4e61; | ||
| text-shadow: 1px 1px 2px rgba(0, 0, 0, 0.3); | ||
| font-size: 14px; | ||
| line-height: 22px; | ||
|
|
||
| outline: none; | ||
| border: 1px solid #e1e7ed; | ||
| border-radius: 3px; | ||
| } |
There was a problem hiding this comment.
The font-size and line-height properties in this block are the same as in the .search-bar__big class. There's no need to repeat them here, they will be inherited from the .search-bar__big class.
src/style.css
Outdated
| .search-bar__small:focus { | ||
| font-size: 14px; | ||
| font-weight: 900; | ||
|
|
||
| background: linear-gradient(180deg, #fff 0%, #f6f6f7 100%); | ||
| border: 1px solid #e1e7ed; | ||
| outline: none; | ||
| box-shadow: none; | ||
| text-shadow: none; | ||
| } |
There was a problem hiding this comment.
The same issue here with the font-size property. It's the same as in the .search-bar__small class, so there's no need to repeat it here.
There was a problem hiding this comment.
Great job so far! 👍 However, there are some issues that need to be addressed before we can approve. Please correct the formatting of the attributes for the input element and add a submit button to the form. Also, remember to adjust the font-family property in your CSS rules. Keep going! 🚀
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| <form class="search-bar"> |
There was a problem hiding this comment.
HTML Content issue: The form tag should have a submit button. Without a submit button, the form will not behave as expected.
| data-qa="big" | ||
| placeholder="Try “Los Angeles“" | ||
| class="search-bar__big input" | ||
| /> |
There was a problem hiding this comment.
HTML Formatting issue: The attributes for the input element are not correctly formatted. Each attribute should start on a new line with 2-space indentation related to the tag. The tag’s closing bracket should be on the same level as the opening one. Please refer to the checklist for the correct format.
| data-qa="small" | ||
| placeholder="Try “Los Angeles“" | ||
| class="search-bar__small input" | ||
| /> |
There was a problem hiding this comment.
HTML Formatting issue: The attributes for the input element are not correctly formatted. Each attribute should start on a new line with 2-space indentation related to the tag. The tag’s closing bracket should be on the same level as the opening one. Please refer to the checklist for the correct format.
| @@ -1 +1,86 @@ | |||
| /* add styles here */ | |||
| @font-face { | |||
| font-family: Avenir, Arial, sans-serif; | |||
There was a problem hiding this comment.
The font-family property inside a @font-face rule doesn't accept a list of fonts. It should contain only the name of the font that's being loaded with the rule. The fallback fonts should be listed in the font-family property in your CSS rules, not in the @font-face rule. For example, you can write it like this: font-family: 'Avenir', Arial, sans-serif; in your CSS rules.
| } | ||
|
|
||
| @font-face { | ||
| font-family: Avenir-Heavy, Arial, sans-serif; |
There was a problem hiding this comment.
The same issue as above. The font-family property inside a @font-face rule should contain only the name of the font that's being loaded with the rule.
src/style.css
Outdated
| body { | ||
| margin: 0; | ||
| padding: 0 8px; | ||
| font-family: Avenir, Arial, Helvetica, sans-serif; |
There was a problem hiding this comment.
This line is where you should be specifying your fallback fonts. If 'Avenir' fails to load, the browser will try to use 'Arial', then 'Helvetica', and finally 'sans-serif' if none of the previous fonts are available. The correct format would be font-family: 'Avenir', 'Avenir-Heavy', Arial, Helvetica, sans-serif;
| } | ||
|
|
||
| body { | ||
| margin: 0; |
There was a problem hiding this comment.
It's a good practice to add a fallback font-family in case the main one doesn't work (from the checklist).
src/style.css
Outdated
| body { | ||
| margin: 0; | ||
| padding: 0 8px; | ||
| font-family: Avenir, Arial, Helvetica, sans-serif; | ||
| } |
There was a problem hiding this comment.
You are styling the body tag directly. It would be better to add a class to the body and style that class instead. Styling elements using tag names can lead to unexpected results when your CSS grows more complex or when you add more HTML to your page (from the checklist).
There was a problem hiding this comment.
Great job on your code! There are a few minor issues that need to be addressed. Please, pay attention to the use of CSS properties and the naming of classes. Also, consider using semantic tags for better readability. Review the comments for more details. Keep learning and improving! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| data-qa="big" | ||
| placeholder="Try “Los Angeles“" | ||
| class="search-bar__big input" | ||
| /> |
There was a problem hiding this comment.
According to the HTML Formatting checklist, it's recommended to add empty lines between multiline sibling blocks of HTML. This will make your code more readable. Please add an empty line between these two input fields.
| <form class="search-bar"> | ||
| <input | ||
| type="text" | ||
| data-qa="big" | ||
| placeholder="Try “Los Angeles“" | ||
| class="search-bar__big input" | ||
| /> | ||
| <input | ||
| type="text" | ||
| data-qa="small" | ||
| placeholder="Try “Los Angeles“" | ||
| class="search-bar__small input" | ||
| /> | ||
| </form> |
There was a problem hiding this comment.
It's recommended to use semantic tags where possible. In this case, you might want to consider wrapping each input field in a div or section tag for better readability and semantics.
| type="text" | ||
| data-qa="big" | ||
| placeholder="Try “Los Angeles“" | ||
| class="search-bar__big input" |
There was a problem hiding this comment.
Remember that class names should represent the meaning of the content, not the styles or tag names. In this case, the class name 'input' doesn't seem to describe the content of the element. Consider renaming it to something more descriptive.
| type="text" | ||
| data-qa="small" | ||
| placeholder="Try “Los Angeles“" | ||
| class="search-bar__small input" |
There was a problem hiding this comment.
The same comment applies to the class name 'input' here. Consider renaming it to something more descriptive.
| @@ -1 +1,85 @@ | |||
| /* add styles here */ | |||
| @font-face { | |||
| font-family: Avenir, Arial, sans-serif; | |||
There was a problem hiding this comment.
In the font-face rule, the font-family property should not include fallback fonts. It should only include the name of the font you're importing. The fallback fonts should be specified where you use the font. (CSS issue)
| } | ||
|
|
||
| @font-face { | ||
| font-family: Avenir-Heavy, Arial, sans-serif; |
There was a problem hiding this comment.
Same issue here with the font-family property in the font-face rule. It should only include the name of the font you're importing. (CSS issue)
| border-radius: 4px; | ||
| box-shadow: 0 1px 8px 0 #3d4e611a; | ||
|
|
||
| font-family: Avenir, Avenir-Heavy, Arial, Helvetica, sans-serif; |
There was a problem hiding this comment.
Here is where you should specify your fallback fonts. However, ensure that the font names match the names specified in the font-face rules. For example, you have Avenir-Heavy as a separate font, so it should not be a fallback for Avenir. (CSS issue)
| background: url('./images/Search.svg') no-repeat 26px center; | ||
|
|
||
| background-size: 19px; | ||
| margin-block: 20px; |
There was a problem hiding this comment.
margin-block is not a standard CSS property. Did you mean to use margin or margin-block-start and margin-block-end? (CSS issue)
| font-weight: 900; | ||
|
|
||
| background: linear-gradient(180deg, #fff 0%, #f6f6f7 100%); | ||
| border: 1px solid #e1e7ed; |
There was a problem hiding this comment.
The border property is repeated in the :focus state. If the border style does not change when the input is focused, there is no need to repeat it. (Code quality issue)
| background: linear-gradient(180deg, #fff 0%, #f6f6f7 100%); | ||
| border: 1px solid #e1e7ed; | ||
| outline: none; | ||
| box-shadow: none; |
There was a problem hiding this comment.
The box-shadow property is set to none in the :focus state. If you do not want a box shadow in the :focus state, there is no need to specify it. (Code quality issue)
❗️ Copy this
Checklistto thePull Requestdescription after links, and put- [x]before each point after you checked it.Typical MistakesfromBEMlesson theory are checked.