first commit #4534
first commit #4534
Conversation
SerhiiUnhurian
commented
Oct 11, 2024
SerhiiUnhurian
commented
- DEMO LINK
- TEST REPORT LINK
There was a problem hiding this comment.
Your work is impressive, but there are a few improvements that need to be made. Please consider the suggestions provided, such as using more descriptive class names, adding fallback fonts, and avoiding the use of a * selector. Also, don't forget to add action and method attributes to your form elements, even if they're not going to be used. 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
| <div class="search-wrapper"> | ||
| <form class="search-bar search-bar--big" data-qa="big"> | ||
| <input | ||
| class="search-bar__input--big" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| </form> | ||
|
|
||
| <input | ||
| type="text" | ||
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| <form class="search-bar search-bar--small" data-qa="small"> | ||
| <input | ||
| class="search-bar__input--small" | ||
| type="text" | ||
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| </form> | ||
| </div> |
There was a problem hiding this comment.
You can use the section or main semantic tag instead of the div to wrap your form elements. This will improve the structure of your HTML and make it more accessible.
src/index.html
Outdated
| <form class="search-bar search-bar--big" data-qa="big"> | ||
| <input | ||
| class="search-bar__input--big" | ||
| type="text" | ||
| data-qa="keypress" | ||
| placeholder="Try “Los Angeles“" | ||
| /> |
There was a problem hiding this comment.
The form elements lack the action and method attributes. Even if they're not going to be used in this case, it's a good practice to include them for a complete form definition.
src/index.html
Outdated
| <form class="search-bar search-bar--small" data-qa="small"> | ||
| <input | ||
| class="search-bar__input--small" | ||
| type="text" | ||
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| /> |
There was a problem hiding this comment.
The form elements lack the action and method attributes. Even if they're not going to be used in this case, it's a good practice to include them for a complete form definition.
src/index.html
Outdated
| <div class="search-wrapper"> | ||
| <form class="search-bar search-bar--big" data-qa="big"> | ||
| <input | ||
| class="search-bar__input--big" |
There was a problem hiding this comment.
The class names search-bar__input--big and search-bar__input--small are not very descriptive. It's better to use class names that describe the meaning of the content, not the styles. For example, you can use search-bar__input-primary and search-bar__input-secondary.
src/index.html
Outdated
| /> | ||
| <form class="search-bar search-bar--small" data-qa="small"> | ||
| <input | ||
| class="search-bar__input--small" |
There was a problem hiding this comment.
The class names search-bar__input--big and search-bar__input--small are not very descriptive. It's better to use class names that describe the meaning of the content, not the styles. For example, you can use search-bar__input-primary and search-bar__input-secondary.
src/style.css
Outdated
| * { | ||
| box-sizing: border-box; | ||
| margin: 0; | ||
| padding: 0; | ||
| font-family: Avenir, sans-serif; | ||
| font-weight: 300; | ||
| } |
There was a problem hiding this comment.
You are using a * selector, which is not recommended due to its impact on performance. Please consider using more specific selectors.
src/style.css
Outdated
| font-family: Avenir, sans-serif; | ||
| font-weight: 300; |
There was a problem hiding this comment.
You should add a fallback font to the font-family property in case Avenir font doesn't load. For example, you can use a generic font family like sans-serif as a fallback.
src/style.css
Outdated
| .search-wrapper { | ||
| display: flex; | ||
| flex-direction: column; | ||
| width: 100%; | ||
| gap: 20px; | ||
| } |
There was a problem hiding this comment.
It is generally not a good idea to fix container size unless there is a specific requirement to do so. Here you have set a width of 100% for the .search-wrapper class. Consider letting the content size dictate the container size to avoid overflow or accidental scroll bar.
There was a problem hiding this comment.
Great job on your HTML! 👍 However, there are a few issues in your CSS that need to be addressed before we can approve this Pull Request. Please ensure the font family is consistent, include a fallback font, and avoid using the * selector due to its impact on performance. Keep up the good work, you're almost there! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/style.css
Outdated
| box-sizing: border-box; | ||
| margin: 0; | ||
| padding: 0; | ||
| font-family: Avenir, sans-serif; |
There was a problem hiding this comment.
The use of the * selector is generally discouraged due to its impact on performance. It would be better to set these styles for specific elements that require them. (See the CSS section of the checklist)
src/style.css
Outdated
| @@ -1 +1,86 @@ | |||
| /* add styles here */ | |||
| @font-face { | |||
| font-family: Avenir, Helvetica, Arial, sans-serif; | |||
There was a problem hiding this comment.
When specifying a font family, it's a good practice to include a fallback font in case the main one doesn't load correctly. In this case, Avenir is not a standard font, so you should include a fallback like sans-serif. (See the CSS section of the checklist)
src/style.css
Outdated
| } | ||
|
|
||
| .search-bar__input:focus { | ||
| font-family: Avenir-Heavy, sans-serif; |
There was a problem hiding this comment.
Here you're changing the font family on focus, which can lead to a jarring experience for the user. It's better to keep the font family consistent across different states.
There was a problem hiding this comment.
Great job on your project! There are a few areas to improve upon, such as avoiding fixed heights on your elements, ensuring your HTML attributes are formatted correctly, and adding empty lines between multiline sibling blocks of HTML for readability. However, these are minor issues and do not hinder the functionality of your code. Keep up the good work and continue to practice and refine your skills. Happy coding! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| </form> | ||
| <form |
There was a problem hiding this comment.
To add some "air" and simplify reading, add an empty line between sibling blocks of HTML. This is especially helpful for multiline blocks. Remember, don't add them between parent and child elements. [Add empty lines between multiline sibling blocks of HTML]
| data-qa="hover" | ||
| placeholder="Try “Los Angeles“" | ||
| /> | ||
| </form> |
There was a problem hiding this comment.
To add some "air" and simplify reading, add an empty line between sibling blocks of HTML. This is especially helpful for multiline blocks. Remember, don't add them between parent and child elements. [Add empty lines between multiline sibling blocks of HTML]
| @@ -17,16 +17,27 @@ | |||
| /> | |||
There was a problem hiding this comment.
The link tag and its attributes are not formatted correctly. 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. [Keep your attributes correctly formatted]
| .search-bar__input { | ||
| display: block; | ||
| width: 100%; | ||
| margin-top: 20px; |
There was a problem hiding this comment.
You have defined a top margin for the .search-bar__input class. As per the checklist, you should be consistent with your margins. Try to add either top or bottom margins, but not both to avoid potential margin collapse.
| } | ||
|
|
||
| body { | ||
| font-family: Avenir, Arial, Helvetica, sans-serif; |
There was a problem hiding this comment.
In your body selector, you've defined a font-family. This is good, but remember to also include a fallback font in case the main one doesn't work. You've already done this correctly here.
| display: block; | ||
| width: 100%; | ||
| margin-top: 20px; | ||
| font-family: Avenir, Arial, Helvetica, sans-serif; |
There was a problem hiding this comment.
You have defined a font-family for the .search-bar__input class. This is unnecessary as the font-family has already been defined in the body selector which will be inherited by all child elements. You can remove this line.
| } | ||
|
|
||
| .search-bar__input--big { | ||
| height: 70px; |
There was a problem hiding this comment.
You've fixed the height of the .search-bar__input--big class. Unless there's a specific requirement for this, it's better to let the content size dictate the height of the container. This helps to avoid overflow or accidental scroll bars.
| } | ||
|
|
||
| .search-bar__input--small { | ||
| height: 42px; |
There was a problem hiding this comment.
Similar to the above comment, you've fixed the height of the .search-bar__input--small class. Again, unless there's a specific requirement for this, it's better to let the content size dictate the height of the container.
